Re: PATCH: Handle ZERO_EXTEND offsettable address

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Sun, Nov 11, 2012 at 7:01 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> This patch also handles SIGN_EXTEND.  Tested on Linux/x32.  OK to
>> install?
>
> I'd cautious here, that's uncharted territory and the SIGN_EXTEND case isn't
> covered by your testing.
>
>> 2012-11-10  H.J. Lu  <hongjiu.lu@intel.com>
>>
>>       PR middle-end/55247
>>       PR middle-end/55259
>>       * emit-rtl.c (adjust_address_1): Handle ZERO_EXTEND and
>>       SIGN_EXTEND.
>
> The new transformation is:
>   (zero_extend ADDR) + CONST_INT -> (zero_extend (ADDR + CONST_INT))
> but in convert_memory_address_addr_space we do the opposite:
>   (zero_extend (ADDR + CONST_INT)) -> (zero_extend ADDR) + CONST_INT
>
> Which one is the canonical form in the end?
>
>> @@ -2109,6 +2111,20 @@ adjust_address_1 (rtx memref, enum machine_mode mode,
>> HOST_WIDE_INT offset, addr = gen_rtx_LO_SUM (address_mode, XEXP (addr, 0),
>>                              plus_constant (address_mode,
>>                                             XEXP (addr, 1), offset));
>> +      /* We permute zero/sign-extension and addition operation only if
>> +      converting the constant does not change it.  */
>> +      else if ((GET_CODE (addr) == ZERO_EXTEND
>> +             || GET_CODE (addr) == SIGN_EXTEND)
>> +            && (x = GEN_INT (offset),
>> +                x == convert_memory_address_addr_space (address_mode,
>> +                                                        x,
>> +                                                        attrs.addrspace)))
>> +     {
>> +       enum rtx_code code = GET_CODE (addr);
>> +       addr = XEXP (addr, 0);
>> +       addr = plus_constant (GET_MODE (addr), addr, offset);
>> +       addr = gen_rtx_fmt_e (code, address_mode, addr);
>> +     }
>
> You need to test the truncation here, not the extension.  Moreover, the
> transformation is valid only under a no-overflow assumption, so I think we
> need to explicitly request pointer_mode (or else restrict the transformation
> to small offsets).
>
>> diff --git a/gcc/recog.c b/gcc/recog.c
>> index ee68e30..a916ef6 100644
>> --- a/gcc/recog.c
>> +++ b/gcc/recog.c
>> @@ -1934,15 +1934,22 @@ int
>>  offsettable_address_addr_space_p (int strictp, enum machine_mode mode, rtx
>> y, addr_space_t as)
>>  {
>> -  enum rtx_code ycode = GET_CODE (y);
>> +  enum rtx_code ycode;
>>    rtx z;
>> -  rtx y1 = y;
>> +  rtx y1;
>>    rtx *y2;
>>    int (*addressp) (enum machine_mode, rtx, addr_space_t) =
>>      (strictp ? strict_memory_address_addr_space_p
>>
>>            : memory_address_addr_space_p);
>>
>>    unsigned int mode_sz = GET_MODE_SIZE (mode);
>>
>> +  /* Allow zero-extended or sign-extended address.  */
>> +  if (GET_CODE (y) == ZERO_EXTEND || GET_CODE (y) == SIGN_EXTEND)
>> +    y = XEXP (y, 0);
>> +
>> +  ycode = GET_CODE (y);
>> +  y1 = y;
>> +
>>    if (CONSTANT_ADDRESS_P (y))
>>      return 1;
>
> That's not fully correct since you don't test the final form of the address.
> You instead need to duplicate the adjust_address_1 change here:
>
>   /* The offset added here is chosen as the maximum offset that
>      any instruction could need to add when operating on something
>      of the specified mode.  We assume that if Y and Y+c are
>      valid addresses then so is Y+d for all 0<d<c.  adjust_address will
>      go inside a LO_SUM here, so we do so as well.  */
>   if (GET_CODE (y) == LO_SUM
>       && mode != BLKmode
>       && mode_sz <= GET_MODE_ALIGNMENT (mode) / BITS_PER_UNIT)
>     z = gen_rtx_LO_SUM (GET_MODE (y), XEXP (y, 0),
>                         plus_constant (GET_MODE (y), XEXP (y, 1),
>                                        mode_sz - 1));
>   else
>     z = plus_constant (GET_MODE (y), y, mode_sz - 1);
>
>
> Adjusted patch attached.
>
> --
> Eric Botcazou

It fixes the problem.  Can you check it in?

Thanks.

-- 
H.J.