[PATCH] Relax check against commuting XOR and ASHIFTRT in combine.c
Alan Lawrence
alan.lawrence@arm.com
Wed Aug 20 10:05:00 GMT 2014
Thanks to Arnaud for confirming that Adacore does not have interest in the
Ada/Alpha combination (https://gcc.gnu.org/ml/gcc-patches/2014-08/msg01832.html).
As per below, I've tested check-ada on x86_64-none-linux-gnu without problems.
Can I say, "ping"? :)
Cheers, Alan
Alan Lawrence wrote:
> Ok, the attached tests are passing on x86_64-none-linux-gnu, aarch64-none-elf,
> arm-none-eabi, and a bunch of smaller platforms for which I've only built a
> stage 1 compiler (i.e. as far as necessary to assemble). That's with either
> change to simplify_shift_const_1.
>
> As to the addition of "result_mode != shift_mode", or removing the whole check
> against XOR - I've now tested the latter:
>
> bootstrapped on x86_64-none-linux-gnu, check-gcc and check-ada;
> bootstrapped on arm-none-linux-gnueabihf;
> bootstrapped on aarch64-none-linux-gnu;
> cross-tested check-gcc on aarch64-none-elf;
> cross-tested on arm-none-eabi;
> (and Uros has bootstrapped on alpha and done a suite of tests, as per
> https://gcc.gnu.org/ml/gcc-testresults/2014-07/msg01236.html).
>
> From a perspective of paranoia, I'd lean towards adding "result_mode !=
> shift_mode", but for neatness removing the whole check against XOR is nicer. So
> I'd defer to the maintainers as to whether one might be preferable to the
> other...(but my unproven suspicion is that the two are equivalent, and no case
> where result_mode != shift_mode is possible!)
>
> --Alan
>
> Alan Lawrence wrote:
>> Thanks for the suggestions! I think I've got a reasonably platform-independent
>> testcase that scans the rtl dump, just trying it on a few more platforms now...
>>
>> As to running on Alpha: bootstrap succeeds, and the regression testsuite doesn't
>> raise any issues (https://gcc.gnu.org/ml/gcc-testresults/2014-07/msg01236.html)
>> - and that's with a more aggressive patch that completely rolls back the
>> original r76965:
>>
>> Index: combine.c
>> ===================================================================
>> --- combine.c (revision 212523)
>> +++ combine.c (working copy)
>> @@ -10218,9 +10218,6 @@
>> if (CONST_INT_P (XEXP (varop, 1))
>> /* We can't do this if we have (ashiftrt (xor)) and the
>> constant has its sign bit set in shift_mode. */
>> - && !(code == ASHIFTRT && GET_CODE (varop) == XOR
>> - && 0 > trunc_int_for_mode (INTVAL (XEXP (varop, 1)),
>> - shift_mode))
>> && (new_rtx = simplify_const_binary_operation
>> (code, result_mode,
>> gen_int_mode (INTVAL (XEXP (varop, 1)), result_mode),
>> @@ -10237,10 +10234,7 @@
>> logical expression, make a new logical expression, and apply
>> the inverse distributive law. This also can't be done
>> for some (ashiftrt (xor)). */
>> - if (CONST_INT_P (XEXP (varop, 1))
>> - && !(code == ASHIFTRT && GET_CODE (varop) == XOR
>> - && 0 > trunc_int_for_mode (INTVAL (XEXP (varop, 1)),
>> - shift_mode)))
>> + if (CONST_INT_P (XEXP (varop, 1)))
>> {
>> rtx lhs = simplify_shift_const (NULL_RTX, code, shift_mode,
>> XEXP (varop, 0), count);
>>
>> I'm testing this version more widely but initial indications are good.
>>
>> However, I've not succeeded in checking Ada on Alpha, as GCC's Ada frontend
>> requires an Ada compiler to bootstrap. So I have to ask: does anyone actually
>> use Ada on Alpha? (And if so, would they please be able to test the above patch?)
>>
>> Moreover, I don't really see we have much reason to believe the check against
>> commuting is required even for Ada/Alpha. GCC's internals have changed
>> substantially in the interim, with the Ada frontend no longer generating RTL
>> directly, as we now have intervening GENERIC/GIMPLE tree stages. Unless there is
>> a logical/bitwise explanation for why the commuting of ashiftrc and xor is
>> unsafe, is the best explanation that the Ada frontend was generating RTL that
>> may have looked OK at the time but we would now consider dubious, malformed, bad?
>>
>> (E.g., these days I don't see how to produce an ashiftrt of one mode containing
>> an XOR of another without an intervening sign_extend, zero_extend or subreg.)
>>
>> --Alan
>>
>> Jeff Law wrote:
>>> On 06/30/14 13:05, Alan Lawrence wrote:
>>>> combine.c includes a check which prevents
>>>>
>>>> (ashiftrt (xor A C2) C1)
>>>>
>>>> from being commuted to
>>>>
>>>> (xor (ashiftrt A C1) (ashiftrt C2 C1))
>>>>
>>>> for constants C1, C2 if C2 has its sign bit set.
>>>>
>>>> Specifically, this prevents (ashiftrt (not A) C1) from being commuted to
>>>>
>>>> (not (ashiftrt A C1))
>>>>
>>>> because the former is rewritten to (ashiftrt (xor A -1) C1) above, with
>>>> a comment /* Make this fit the case below. */ - which it no longer does.
>>>>
>>>> If result_mode == shift_mode, I can see no reason to prevent this
>>>> normalisation (indeed, I'm not sure I can see any reason to prevent it
>>>> even if result_mode != shift_mode - but I've not managed to produce such
>>>> a case in my own testing, as there are always intervening subreg's or
>>>> sign_extend's, or to build a toolchain on which to reproduce the
>>>> original bug, so I'm being cautious). Hence this patch allows
>>>> commutation if the two modes are equal.
>>>>
>>>> As an illustrative example, on AArch64, without this patch, compiling
>>>> this snippet of the current arm_neon.h:
>>>>
>>>> typedef long long int int64x1_t __attribute__ ((__vector_size__((8))));
>>>> int64x1 vcgez_s64(int64x1_t a)
>>>> {
>>>> return (int64x1_t) {a >=0 ? -1 : 0};
>>>> }
>>>>
>>>> produces a sequence involving a logical not (mvn) followed by asr (plus
>>>> some inter-bank moves), as the combiner takes (ashiftrt (not x) 63),
>>>> "simplifies", and fails to match the resulting RTL
>>>>
>>>> (set (reg:DI 78 [ D.2593 ])
>>>> (ashiftrt:DI (xor:DI (reg:DI 0 x0 [ aD.2565 ])
>>>> (const_int -1 [0xffffffffffffffff]))
>>>> (const_int 63 [0x3f])))
>>>>
>>>> However, with this patch, the combiner simplifies to
>>>>
>>>> (set (reg:DI 84 [ D.2604 ])
>>>> (neg:DI (ge:DI (reg:DI 32 v0 [ aD.2569 ])
>>>> (const_int 0 [0]))))
>>>>
>>>> which matches a pattern to output the desired cmge instruction.
>>>>
>>>> (For the curious: the check against commutation was added in January
>>>> 2004, r76965, https://gcc.gnu.org/ml/gcc-patches/2004-01/msg03406.html -
>>>> however, I've not been able to build an ADA toolchain of that era, or an
>>>> Alpha/VMS toolchain, on which to reproduce the bug; if anyone has such
>>>> and is willing and able to investigate, by all means!)
>>>>
>>>> Testing:
>>>>
>>>> aarch64-none-elf: check-gcc (cross-tested)
>>>> x86-none-linux-gnu: bootstrapped; check-gcc, check-ada, check-g++
>>>> arm-none-linux-gnueabi: bootstrapped
>>>> arm-none-eabi: check-gcc (cross-tested)
>>>>
>>>>
>>>> gcc/ChangeLog:
>>>>
>>>> * combine.c (simplify_shift_const_1): Allow commuting ASHIFTRT and XOR
>>>> if same mode.
>>> You'll want to use your sample code from above to create a testcase.
>>> You can either dump the RTL and search it, or scan the assembly code.
>>>
>>> Look in gcc/testsuite/gcc.target/arm for examples.
>>>
>>> Given the original problem which prompted Kenner to make this change was
>>> Ada related on the Alpha, you might ping rth and/or uros to see if they
>>> could do a "quick" regression of those platforms with Ada enabled.
>>>
>>> I'm naturally hesitant to approve since this was something pretty Kenner
>>> explicitly tried to avoid AFAICT. Kenner wasn't ever known for trying
>>> to paper over problems -- if he checked in a change like this, much more
>>> likely than not it was well thought out and analyzed. He also probably
>>> knew combine.c better than anyone at that time (possibly even still true
>>> today).
>>>
>>>
>>> Jeff
>>>
>>>
>>>
>>
>>
More information about the Gcc-patches
mailing list