This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: RFC: arm_legitimate_address_p change for minipool constants


On Thu, Jan 22, 2004 at 12:47:44PM +0000, Richard Earnshaw wrote:
> > I have a testcase that, after backporting Richard's patch to constrain MEM
> > operands more strictly after reload to the 3.3 branch, started ICEing. 
> > Unfortunately I could not reproduce the problem with HEAD, but I'm pretty
> > sure it's still there - I just can't get combine to generate the problematic
> > zero_extend any more.
> > 
> > The test comes from glib and is a 64-bit byte swap.  Here's the troublesome
> > insn before arm_reorg:
> > 
> > (insn:HI 49 193 209 2 0x4036d2ec (set (reg:DI 3 r3 [77])
> >         (zero_extend:DI (mem/u/f:QI (symbol_ref/u:SI ("*.LC1")) [0 S1 A8]))) 146 {zero_extendqidi2}
> >     (insn_list:REG_DEP_ANTI 208 (insn_list:REG_DEP_ANTI 207 (insn_list:REG_DEP_OUTPUT 61 (nil))))
> >     (expr_list:REG_UNUSED (reg:SI 4 r4)
> >         (nil)))
> > 
> > After arm_reorg:
> > 
> > (insn:HI 49 193 209 0x4036d2ec (set (reg:DI 3 r3 [77])
> >         (zero_extend:DI (mem:QI (const (plus (label_ref 249)
> >                         (const_int 52 [0x34]))) [0 S1 A8]))) 146 {zero_extendqidi2}
> >     (insn_list:REG_DEP_ANTI 208 (insn_list:REG_DEP_ANTI 207 (insn_list:REG_DEP_OUTPUT 61 (nil))))
> >     (expr_list:REG_UNUSED (reg:SI 4 r4)
> >         (nil)))
> > 
> > Notice that the mem is QImode.  This is rejected:
> >   else if (GET_MODE_SIZE (MODE) >= 4 && reload_completed                \
> >            && (GET_CODE (X) == LABEL_REF                                \
> >                || (GET_CODE (X) == CONST                                \
> >                    && GET_CODE (XEXP ((X), 0)) == PLUS                  \
> >                    && GET_CODE (XEXP (XEXP ((X), 0), 0)) == LABEL_REF   \
> >                    && GET_CODE (XEXP (XEXP ((X), 0), 1)) == CONST_INT)))\
> >     goto LABEL;                                                         \
> > 
> > If I remove the GET_MODE_SIZE check, the test passes.  I did some
> > archaeology, to the limited extent I can from the gcc.gnu.org repository;
> > the check has been there since revision 1.1.  I'm guessing it came in a
> > patch from Richard Earnshaw in 1995 based on FSFChangeLog.11.  I can not see
> > the reason for it, and the comment above it in arm_legitimate_address_p in
> > HEAD explains all the other terms of the condition except for that one.
> > 
> 
> You don't really expect me to be able to remember precisely why I put that 
> test in over 8 years ago, surely :-)

Of course not.  I was hoping for a hunch, though :)

> I suspect that it was because I thought that such an access would ever 
> really need to reference the constant pool (a byte-load of an immediate 
> can just be replaced by a mov of the immediate) -- however, I'm no-longer 
> convinced of that case: if code plays funny games with casts, then it is 
> possible to force the compiler into making such a reference by accessing a 
> subreg of a label or symbol.

Hmm, this makes sense.

> > I have just started regression testing the following patch; OK if it passes?
> > Can anyone think of a reason for the test to have been originally added?
> 
> I'm not sure if it's precisely the same problem, but have you looked at 
> pulling up revision 1.302 of arm.c?

I thought about it and convinced myself it couldn't fix the problem. 
Then I tried it today.  Something surprising (to me) happens...

We start with:
  (mem/u/f:QI (symbol_ref/u:SI ("*.LC1")) [0 S1 A8])

Calling avoid_constant_pool_reference returns:
  (const_int -89 [0xffffffa7])

Ideal!  But this_fix->value is never used when fixing up the
instruction, only when looking in the minipool.  Ideal would be to, at
this point, replace the MEM with a const_int instead of fixing up its
minipool reference, if CONST_OK_FOR_LETTER_P.

My conclusions:
  - I think the change to arm_legitimate_address_p is correct, since
    the addressing mode is valid, and for Thumb/HImode replacing by
    a constant mov is not possible.
  - The -89 in this case should be transformed into a mov, but it
    should probably not be happening in arm_reorg.  I don't know where
    it should be happening.

> Erm, 1.303.2 is the csl-arm branch, not gcc-3.3.  What exactly are you 
> asking for approval for?

Sorry, I was just being sloppy.  I'm asking for approval for HEAD and
3.4 - you're right that I diffed csl_arm_branch, but the relevant code
is the same on HEAD.  This is not a problem in 3.3 unless you backport
the strict constraints patch, which I would not recommend for the 3.3
branch.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]