Re: PING: PATCH [4/n]: Prepare x32: Permute the conversion and addition if one operand is a constant

[Andrew Pinski <pinskia at gmail dot com>]

On Wed, Jul 13, 2011 at 9:39 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Wed, Jul 13, 2011 at 9:13 AM, Paolo Bonzini <bonzini@gnu.org> wrote:
>> On 07/11/2011 05:54 PM, H.J. Lu wrote:
>>>
>>> The key is the
>>> >
>>> >     XEXP (x, 1) == convert_memory_address_addr_space
>>> >                    (to_mode, XEXP (x, 1), as)
>>> >
>>> >  test.  It ensures basically that the constant has 31-bit precision,
>>> > because
>>> >  otherwise the constant would change from e.g. (const_int -0x7ffffffc)
>>> > to
>>> >  (const_int 0x80000004) when zero-extending it from SImode to DImode.
>>> >
>>> >  But I'm not sure it's safe.  You have,
>>> >
>>> >    (zero_extend:DI (plus:SI FOO:SI) (const_int Y))
>>> >
>>> >  and you want to convert it to
>>> >
>>> >    (plus:DI FOO:DI (zero_extend:DI (const_int Y)))
>>> >
>>> >  (where the zero_extend is folded).  Ignore that FOO is a SYMBOL_REF
>>> > (this
>>> >  piece of code does not assume anything about its shape); if FOO ==
>>> >  0xfffffffc and Y = 8, the result will be respectively 0x4 (valid) and
>>> >  0x100000004 (invalid).
>>>
>>> This example contradicts what you said above "It ensures basically that
>>> the
>>> constant has 31-bit precision".
>>
>> Why?  Certainly Y = 8 has 31-bit (or less) precision.  So it has the same
>> representation in SImode and DImode, and the test above on XEXP (x, 1)
>> succeeds.
>
> And then we permute conversion and addition, which leads to the issue you
> raised above.  In another word, the current code permutes conversion
> and addition.
> It leads to different values in case of symbol (0xfffffffc) + 8.
> Basically the current
> test for 31-bit (or less) precision is bogus.  The real question is
> for a address
> computation, A + B, if address wrap-around is supported in
> convert_memory_address_addr_space.

Unless the code has already reassociated the additions already.
Like in the AARCH64 ILP32 case:

(plus:SI (plus:SI (mult:SI (reg/v:SI 80 [ b ])
            (const_int -4 [0xfffffffffffffffc]))
        (subreg/s/u:SI (reg/v/f:DI 79 [ a ]) 0))
    (const_int -1073742592 [0xffffffffbffffd00]))

The Tree level is correct in that it did not reassociate the addition
but the RTL level ignores that.

So this patch is invalid and incorrect unless you know the non
constant part of the addition is a pointer (which is not the case
here).

Thanks,
Andrew Pinski



>
>>> >  What happens if you just return NULL instead of the assertion (good
>>> > idea
>>> >  adding it!)?
>>> >
>>> >  Of course then you need to:
>>> >
>>> >  1) check the return values of convert_memory_address_addr_space_1, and
>>> >  propagate NULL up to simplify_unary_operation;
>>> >
>>> >  2) check in simplify-rtx.c whether the return value of
>>> >  convert_memory_address_1 is NULL, and only return if the return value
>>> > is not
>>> >  NULL.  This is not yet necessary (convert_memory_address is the last
>>> >  transformation for both SIGN_EXTEND and ZERO_EXTEND) but it is better
>>> > to
>>> >  keep code clean.
>>>
>>> I will give it a try.
>>
>> Thanks, did you get any result?  There's no "I think" in this code.  So even
>> if I cannot approve it, I'd really like to see a version that I understand
>> and that is clearly conservative, if it works.
>>
>
> I opened a new bug:
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49721
>
> My current code looks like:
>
>    case CONST:
>       temp = convert_memory_address_addr_space_1 (to_mode, XEXP (x, 0),
>                                                   as, no_emit,
>                                                   ignore_address_wrap_around);
>       return temp ? gen_rtx_CONST (to_mode, temp) : temp;
>       break;
>
>     case PLUS:
>     case MULT:
>       /* For addition we can safely permute the conversion and addition
>          operation if one operand is a constant, address wrap-around
>          is ignored and we are using a ptr_extend instruction or
>          zero-extending (POINTERS_EXTEND_UNSIGNED != 0).  We can always
>          safely permute them if we are making the address narrower.  */
>       if (GET_MODE_SIZE (to_mode) < GET_MODE_SIZE (from_mode)
>           || (GET_CODE (x) == PLUS
>               && CONST_INT_P (XEXP (x, 1))
>               && (POINTERS_EXTEND_UNSIGNED != 0
>                   && ignore_address_wrap_around)))
>         return gen_rtx_fmt_ee (GET_CODE (x), to_mode,
>                                convert_memory_address_addr_space_1
>                                  (to_mode, XEXP (x, 0), as, no_emit,
>                                   ignore_address_wrap_around),
>                                XEXP (x, 1));
>       break;
>
> --
> H.J.