Re: [PATCH] Relax check against commuting XOR and ASHIFTRT in combine.c

[Alan Lawrence <alan.lawrence@arm.com>]

[-- Attachment #1: Type: text/plain, Size: 7815 bytes --]

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
>>
>>
>>
> 
> 
> 
> 

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: combrtx_tests.patch --]
[-- Type: text/x-patch; name=combrtx_tests.patch, Size: 1929 bytes --]

diff --git a/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c b/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
new file mode 100644
index 0000000000000000000000000000000000000000..90e64fd10dc358f10ad03a90041605bc3ccb7011
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
@@ -0,0 +1,18 @@
+/* { dg-do compile {target sparc64*-*-* aarch64*-*-* x86_64-*-* powerpc64*-*-*} } */
+/* { dg-options "-O2 -fdump-rtl-combine-all" } */
+
+typedef long long int int64_t;
+
+int64_t
+foo (int64_t a)
+{
+  return (~a) >> 63;
+}
+
+/* The combine phase will try to combine not & ashiftrt, and
+   combine_simplify_rtx should transform (ashiftrt (not x) 63)
+   to (not (ashiftrt x 63)) and then to (neg (ge x 0)). We look for
+   the *attempt* to match this RTL pattern, regardless of whether an
+   actual insn may be found on the platform.  */
+/* { dg-final { scan-rtl-dump "\\(neg:DI \\(ge:DI" "combine" } } */
+/* { dg-final { cleanup-rtl-dump "combine" } } */
diff --git a/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c b/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
new file mode 100644
index 0000000000000000000000000000000000000000..fd6827caed230ea5dd2d6ec4431b11bf826531ea
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
@@ -0,0 +1,18 @@
+/* { dg-do compile {target arm*-*-* i?86-*-* powerpc-*-* sparc-*-*} } */
+/* { dg-options "-O2 -fdump-rtl-combine-all" } */
+
+typedef long int32_t;
+
+int32_t
+foo (int32_t a)
+{
+  return (~a) >> 31;
+}
+
+/* The combine phase will try to combine not & ashiftrt, and
+   combine_simplify_rtx should transform (ashiftrt (not x) 31)
+   to (not (ashiftrt x 63)) and then to (neg (ge x 0)). We look for
+   the *attempt* to match this RTL pattern, regardless of whether an
+   actual insn may be found on the platform.  */
+/* { dg-final { scan-rtl-dump "\\(neg:SI \\(ge:SI" "combine" } } */
+/* { dg-final { cleanup-rtl-dump "combine" } } */