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

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

[Alan Lawrence]

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.

---------------------------------
diff --git a/gcc/combine.c b/gcc/combine.c
index 4e7ef55..29a9fc8 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -10219,6 +10219,7 @@ simplify_shift_const_1 (enum rtx_code code, enum machine
               /* 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
+                  && result_mode != shift_mode
                    && 0 > trunc_int_for_mode (INTVAL (XEXP (varop, 1)),
                                               shift_mode))
               && (new_rtx = simplify_const_binary_operation
@@ -10239,6 +10240,7 @@ simplify_shift_const_1 (enum rtx_code code, enum machine
              for some (ashiftrt (xor)).  */
           if (CONST_INT_P (XEXP (varop, 1))
              && !(code == ASHIFTRT && GET_CODE (varop) == XOR
+                 && result_mode != shift_mode
                   && 0 > trunc_int_for_mode (INTVAL (XEXP (varop, 1)),
                                              shift_mode)))

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

[Jeff Law]

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

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

[Alan Lawrence]

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

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

[Alan Lawrence]

[-- 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" } } */

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

[Alan Lawrence]

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

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

[Jeff Law]

On 07/17/14 10:56, 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!)
So first, whether or not someone cares about Alpha-VMS isn't the issue 
at hand.  It's whether or not the new code is correct or not. 
Similarly the fact that the code generation paths are radically 
different now when compared to 2004 and we can't currently trigger the 
cases where the modes are different isn't the issue, again, it's whether 
or not your code is correct or not.

I think the key is to look at try_widen_shift_mode and realize that it 
can return a mode larger than the original mode of the operations. 
However, it only does so when it presented with a case where it knows 
the sign bit being shifted in from the left will be the same as the sign 
bit in the original mode.

In the case of an XOR when the sign bit set in shift_mode, that's not 
going to be the case.  We would violate the assumption made when we 
decided to widen the shift to shift_mode.

So your relaxation is safe when shift_mode == result_mode, but unsafe 
otherwise -- even though we don't currently have a testcase for the 
shift_mode != result_mode case, we don't want to break that.

So your updated patch is correct.  However, I would ask that you make 
one additional change.  Namely the comment before the two fragments of 
code you changed needs updating.  Something like

"... and the constant has its sign bit set in shift_mode and shift_mode
  is different than result_mode".

The 2nd comment should have similar clarifying comments.

You should also include your testcase for a cursory review.

So with the inclusion of the testcase and updated comments, we should be 
good to go.  However, please post the final patch for that cursory 
review of the testcase.

jeff

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

[Alan Lawrence]

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

Thanks for the reply - and the in-depth investigation. I agree that the
correctness of the compiler is critical rather than particular platforms such as
Ada / Alpha.

Moreover, I think we both agree that if result_mode==shift_mode, the
transformation is correct. "Just" putting that check in, achieves what I'm
trying for here, so I'd be happy to go with the attached patch and call it a
day. However, I'm a little concerned about the other cases - i.e. where
shift_mode is wider than result_mode.

If I understand correctly (and I'm not sure about that, let's see how far I
get), this means we'll perform the shift in (say) DImode, when we're only really
concerned about the lower (say) 32-bits (for an originally-SImode shift).
try_widen_shift_mode will in this case check that the result of the operation
*inside* the shift (in our case an XOR) has 33 sign bit copies (via
num_sign_bit_copies), i.e. that its *top* 32-bits are all equal to the original
SImode sign bit. <count> of these bits may be fed into the top of the desired
SImode result by the DImode shift. Right so far?

AFAICT, num_sign_bit_copies for an XOR, conservatively returns the minimum of
the num_sign_bit_copies of its two operands. I'm not sure whether this is
behaviour we should rely on in its callers, or for the sake of abstraction we
should treat num_sign_bit_copies as a black box (which does what it says on the,
erm, tin).

If the former, doesn't having num_sign_bit_copies >= the difference in size
between result_mode and shift_mode, of both operands to the XOR, guarantee
safety of the commutation (whether the constant is positive or negative)? We
could perform the shift (in the larger mode) on both of the XOR operands safely,
then XOR together their lower parts.

If, however, we want to play safe and ensure that we deal safely with any XOR
whose top (mode size difference + 1) bits were the same, then I think the
restriction that the XOR constant is positive is neither necessary nor
sufficient; rather (mirroring try_widen_shift_mode) I think we need that
num_sign_bit_copies of the constant in shift_mode, is more than the size
difference between result_mode and shift_mode.

Hmmm. I might try that patch at some point, I think it is the right check to
make. (Meta-comment: this would be *so*much* easier if we could write unit tests
more easily!) In the meantime I'd be happy to settle for the attached...

(tests are as they were posted
https://gcc.gnu.org/ml/gcc-patches/2014-07/msg01233.html .)

--Alan

Jeff Law wrote:
> On 07/17/14 10:56, 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!)
> So first, whether or not someone cares about Alpha-VMS isn't the issue 
> at hand.  It's whether or not the new code is correct or not. 
> Similarly the fact that the code generation paths are radically 
> different now when compared to 2004 and we can't currently trigger the 
> cases where the modes are different isn't the issue, again, it's whether 
> or not your code is correct or not.
> 
> I think the key is to look at try_widen_shift_mode and realize that it 
> can return a mode larger than the original mode of the operations. 
> However, it only does so when it presented with a case where it knows 
> the sign bit being shifted in from the left will be the same as the sign 
> bit in the original mode.
> 
> In the case of an XOR when the sign bit set in shift_mode, that's not 
> going to be the case.  We would violate the assumption made when we 
> decided to widen the shift to shift_mode.
> 
> So your relaxation is safe when shift_mode == result_mode, but unsafe 
> otherwise -- even though we don't currently have a testcase for the 
> shift_mode != result_mode case, we don't want to break that.
> 
> So your updated patch is correct.  However, I would ask that you make 
> one additional change.  Namely the comment before the two fragments of 
> code you changed needs updating.  Something like
> 
> "... and the constant has its sign bit set in shift_mode and shift_mode
>   is different than result_mode".
> 
> The 2nd comment should have similar clarifying comments.
> 
> You should also include your testcase for a cursory review.
> 
> So with the inclusion of the testcase and updated comments, we should be 
> good to go.  However, please post the final patch for that cursory 
> review of the testcase.
> 
> jeff
> 
> 
> 

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

diff --git a/gcc/combine.c b/gcc/combine.c
index 0ec7f852c47dc2e90d70210b4b7bc8350806d41d..3517479cf879219bc1012b7379b1f495c939c2f4 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -10257,8 +10257,10 @@ simplify_shift_const_1 (enum rtx_code code, enum machine_mode result_mode,
 
 	  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.  */
+		 constant has its sign bit set in shift_mode with shift_mode
+		 wider than result_mode.  */
 	      && !(code == ASHIFTRT && GET_CODE (varop) == XOR
+		   && result_mode != shift_mode
 		   && 0 > trunc_int_for_mode (INTVAL (XEXP (varop, 1)),
 					      shift_mode))
 	      && (new_rtx = simplify_const_binary_operation
@@ -10275,10 +10277,12 @@ simplify_shift_const_1 (enum rtx_code code, enum machine_mode result_mode,
 
 	  /* If we can't do that, try to simplify the shift in each arm of the
 	     logical expression, make a new logical expression, and apply
-	     the inverse distributive law.  This also can't be done
-	     for some (ashiftrt (xor)).  */
+	     the inverse distributive law.  This also can't be done for
+	     (ashiftrt (xor)) where we've widened the shift and the constant
+	     changes the sign bit.  */
 	  if (CONST_INT_P (XEXP (varop, 1))
 	     && !(code == ASHIFTRT && GET_CODE (varop) == XOR
+		  && result_mode != shift_mode
 		  && 0 > trunc_int_for_mode (INTVAL (XEXP (varop, 1)),
 					     shift_mode)))
 	    {
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" } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/simd/int_comparisons_1.c b/gcc/testsuite/gcc.target/aarch64/simd/int_comparisons_1.c
index cb0f4a04c0fb5f2c93064c47a141556f7fd0f89a..f2c55922f18c0e6840c403f419fe4a98a15e5f97 100644
--- a/gcc/testsuite/gcc.target/aarch64/simd/int_comparisons_1.c
+++ b/gcc/testsuite/gcc.target/aarch64/simd/int_comparisons_1.c
@@ -30,18 +30,16 @@
 /* Comparisons against immediate zero, on the 8 signed integer types only.  */
 
 /* { dg-final { scan-assembler-times "\[ \t\]cmge\[ \t\]+v\[0-9\]+\.\[0-9\]+\[bshd\],\[ \t\]*v\[0-9\]+\.\[0-9\]+\[bshd\],\[ \t\]*#?0" 7 } } */
-/*  For int64_t and int64x1_t, combine_simplify_rtx failure of
-    https://gcc.gnu.org/ml/gcc/2014-06/msg00253.html
-    prevents generation of cmge....#0, instead producing mvn + sshr.  */
-/* { #dg-final { scan-assembler-times "\[ \t\]cmge\[ \t\]+d\[0-9\]+,\[ \t\]*d\[0-9\]+,\[ \t\]*#?0" 2 } } */
+/* { dg-final { scan-assembler-times "\[ \t\]cmge\[ \t\]+d\[0-9\]+,\[ \t\]*d\[0-9\]+,\[ \t\]*#?0" 2 } } */
 /* { dg-final { scan-assembler-times "\[ \t\]cmgt\[ \t\]+v\[0-9\]+\.\[0-9\]+\[bshd\],\[ \t\]*v\[0-9\]+\.\[0-9\]+\[bshd\],\[ \t\]*#?0" 7 } } */
 /* { dg-final { scan-assembler-times "\[ \t\]cmgt\[ \t\]+d\[0-9\]+,\[ \t\]*d\[0-9\]+,\[ \t\]*#?0" 2 } } */
 /* { dg-final { scan-assembler-times "\[ \t\]cmle\[ \t\]+v\[0-9\]+\.\[0-9\]+\[bshd\],\[ \t\]*v\[0-9\]+\.\[0-9\]+\[bshd\],\[ \t\]*#?0" 7 } } */
 /* { dg-final { scan-assembler-times "\[ \t\]cmle\[ \t\]+d\[0-9\]+,\[ \t\]*d\[0-9\]+,\[ \t\]*#?0" 2 } } */
 /* { dg-final { scan-assembler-times "\[ \t\]cmlt\[ \t\]+v\[0-9\]+\.\[0-9\]+\[bshd\],\[ \t\]*v\[0-9\]+\.\[0-9\]+\[bshd\],\[ \t\]*#?0" 7 } } */
 /* For int64_t and int64x1_t, cmlt ... #0 and sshr ... #63 are equivalent,
-   so allow either.  cmgez issue above results in extra 2 * sshr....63.  */
-/* { dg-final { scan-assembler-times "\[ \t\](?:cmlt|sshr)\[ \t\]+d\[0-9\]+,\[ \t\]*d\[0-9\]+,\[ \t\]*#?(?:0|63)" 4 } } */
+   so allow either.  */
+/* { dg-final { scan-assembler-times "\[ \t\](?:cmlt|sshr)\[ \t\]+d\[0-9\]+,\[ \t\]*d\[0-9\]+,\[ \t\]*#?(?:0|63)" 2 } } */
 
 // All should have been compiled into single insns without inverting result:
 /* { dg-final { scan-assembler-not "\[ \t\]not\[ \t\]" } } */
+/* { dg-final { scan-assembler-not "\[ \t\]mvn\[ \t\]" } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/singleton_intrinsics_1.c b/gcc/testsuite/gcc.target/aarch64/singleton_intrinsics_1.c
index 8a8272ba48eedc12b56357bf4073dda791fb10f9..4a0934b01f9442b7f1324a1f4528d45022daf9b8 100644
--- a/gcc/testsuite/gcc.target/aarch64/singleton_intrinsics_1.c
+++ b/gcc/testsuite/gcc.target/aarch64/singleton_intrinsics_1.c
@@ -57,8 +57,7 @@ test_vcle_s64 (int64x1_t a, int64x1_t b)
   return vcle_s64 (a, b);
 }
 
-/* Idiom recognition will cause this testcase not to generate
-   the expected cmge instruction, so do not check for it.  */
+/* { dg-final { scan-assembler-times "\\tcmge\\td\[0-9\]+, d\[0-9\]+, #?0" 1 } } */
 
 uint64x1_t
 test_vcgez_s64 (int64x1_t a)
@@ -236,8 +235,8 @@ test_vrshl_u64 (uint64x1_t a, int64x1_t b)
   return vrshl_u64 (a, b);
 }
 
-/* { dg-final { scan-assembler-times "\\tsshr\\td\[0-9\]+" 3 } } */
-/* Idiom recognition compiles vcltz and vcgez to sshr rather than cmlt/cmge.  */
+/* For int64x1_t, sshr...#63 is output instead of the equivalent cmlt...#0.  */
+/* { dg-final { scan-assembler-times "\\tsshr\\td\[0-9\]+" 2 } } */
 
 int64x1_t
 test_vshr_n_s64 (int64x1_t a)

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

[Andreas Schwab]

Alan Lawrence <alan.lawrence@arm.com> writes:

> 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*-*-*} } */

You should check for lp64 instead of matching 64 in target names, to
reject -m32.

> 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-*-*} } */

Likewise, using ilp32 to reject -m64.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

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

[Rainer Orth]

Andreas Schwab <schwab@linux-m68k.org> writes:

> Alan Lawrence <alan.lawrence@arm.com> writes:
>
>> 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*-*-*} } */
>
> You should check for lp64 instead of matching 64 in target names, to
> reject -m32.
>
>> 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-*-*} } */
>
> Likewise, using ilp32 to reject -m64.

Right, the current target lists are simply bogus on biarch targets.

Alan, what's the reasoning behind your current target lists here?  Any
reason the test couldn't work elsewhere?  If not, it would be way better
to introduce a corresponding effective-target keyword than listing
particular targets without explanation.

This needs to be fixed: the issue is knowns for three weeks now and
causes testsuite noise on many platforms.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

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

[Alan Lawrence]

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

Mmmm, I've made a few attempts at filtering according to LP64 and ILP32, but not 
managed to get anything working so far (that is, I've ended up with the test not 
being executed on platforms where it should)....ah, I see now where I've been 
going wrong, patch attached.

The original intent was pretty much to execute the test on everything with the 
appropriate word size, i.e. where a 64-bit comparison would be done in 64 bits 
rather than emulated in 2*32; and for 32-bit where that was not sign-extended to 
64 (or some other such problem). The architectures I wrote in the file, were 
those on which I tested the rtl dump, excluding some archs where you get (neg 
(lt 0 x)) rather than (neg (ge x 0)); but the latter really shouldn't be a 
problem, it should be possible to use a regex matching either form, and then 
drop the target selection.

However, as a quick first step, does adding the ilp32 / lp64 (and keeping the 
architectures list for now) solve the immediate problem? Patch attached, OK for 
trunk?

gcc/testsuite/ChangeLog:

	* gcc.dg/combine_ashiftrt_1.c: require-effective-target LP64
	* gcc.dg/combine_ashiftrt_2.c: require-effective-target ILP32

--Alan
________________________________________
From: Rainer Orth [ro@cebitec.uni-bielefeld.de]
Sent: 23 October 2014 14:10
To: Andreas Schwab
Cc: Alan Lawrence; Jeff Law; gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] Relax check against commuting XOR and ASHIFTRT in combine.c

Andreas Schwab <schwab@linux-m68k.org> writes:

> Alan Lawrence <alan.lawrence@arm.com> writes:
>
>> 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*-*-*} } */
>
> You should check for lp64 instead of matching 64 in target names, to
> reject -m32.
>
>> 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-*-*} } */
>
> Likewise, using ilp32 to reject -m64.

Right, the current target lists are simply bogus on biarch targets.

Alan, what's the reasoning behind your current target lists here?  Any
reason the test couldn't work elsewhere?  If not, it would be way better
to introduce a corresponding effective-target keyword than listing
particular targets without explanation.

This needs to be fixed: the issue is knowns for three weeks now and
causes testsuite noise on many platforms.

         Rainer

--
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

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

commit 43e8585f475dff386d245cb150940755cd9b43d9
Author: Alan Lawrence <alan.lawrence@arm.com>
Date:   Thu Oct 23 17:41:28 2014 +0100

    Add ILP32 / LP64

diff --git a/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c b/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
index 90e64fd..cb669c9 100644
--- a/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
+++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
@@ -1,4 +1,5 @@
 /* { dg-do compile {target sparc64*-*-* aarch64*-*-* x86_64-*-* powerpc64*-*-*} } */
+/* { dg-require-effective-target lp64 } */
 /* { dg-options "-O2 -fdump-rtl-combine-all" } */
 
 typedef long long int int64_t;
diff --git a/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c b/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
index fd6827c..6bd6f2f 100644
--- a/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
+++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
@@ -1,4 +1,5 @@
 /* { dg-do compile {target arm*-*-* i?86-*-* powerpc-*-* sparc-*-*} } */
+/* { dg-require-effective-target ilp32} */
 /* { dg-options "-O2 -fdump-rtl-combine-all" } */
 
 typedef long int32_t;

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

[Rainer Orth]

Alan Lawrence <alan.lawrence@arm.com> writes:

> Mmmm, I've made a few attempts at filtering according to LP64 and ILP32,
> but not managed to get anything working so far (that is, I've ended up with
> the test not being executed on platforms where it should)....ah, I see now
> where I've been going wrong, patch attached.
>
> The original intent was pretty much to execute the test on everything with
> the appropriate word size, i.e. where a 64-bit comparison would be done in
> 64 bits rather than emulated in 2*32; and for 32-bit where that was not
> sign-extended to 64 (or some other such problem). The architectures I wrote
> in the file, were those on which I tested the rtl dump, excluding some
> archs where you get (neg (lt 0 x)) rather than (neg (ge x 0)); but the
> latter really shouldn't be a problem, it should be possible to use a regex
> matching either form, and then drop the target selection.
>
> However, as a quick first step, does adding the ilp32 / lp64 (and keeping
> the architectures list for now) solve the immediate problem? Patch
> attached, OK for trunk?

No, as I said this is wrong for biarch targets like sparc and i386.

> gcc/testsuite/ChangeLog:
>
> 	* gcc.dg/combine_ashiftrt_1.c: require-effective-target LP64
> 	* gcc.dg/combine_ashiftrt_2.c: require-effective-target ILP32

Nit: write this as e.g. "Require lp64." 

> diff --git a/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c b/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
> index 90e64fd..cb669c9 100644
> --- a/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
> +++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
> @@ -1,4 +1,5 @@
>  /* { dg-do compile {target sparc64*-*-* aarch64*-*-* x86_64-*-* powerpc64*-*-*} } */

This should be something like 

  { target aarch64*-*-* i?86-*-* powerpc*-*-* sparc*-*-* x86_64-*-* }

E.g. sparc-sun-solaris2.11 with -m64 is lp64, but would be excluded by
your target list.  Keep the list sorted alphabetically and best add an
explanation so others know what those targets have in common.

> +/* { dg-require-effective-target lp64 } */
>  /* { dg-options "-O2 -fdump-rtl-combine-all" } */
>  
>  typedef long long int int64_t;
> diff --git a/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c b/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
> index fd6827c..6bd6f2f 100644
> --- a/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
> +++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
> @@ -1,4 +1,5 @@
>  /* { dg-do compile {target arm*-*-* i?86-*-* powerpc-*-* sparc-*-*} } */

Same here:

  { target arm*-*-* i?86-*-* powerpc*-*-* sparc*-*-* x86_64-*-* }

> +/* { dg-require-effective-target ilp32} */
>  /* { dg-options "-O2 -fdump-rtl-combine-all" } */
>  
>  typedef long int32_t;

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

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

[Alan Lawrence]

Rainer Orth wrote:
>> However, as a quick first step, does adding the ilp32 / lp64 (and keeping
>> the architectures list for now) solve the immediate problem? Patch
>> attached, OK for trunk?
> 
> No, as I said this is wrong for biarch targets like sparc and i386.

When you say no this does not solve the immediate problem, are you saying that 
you are (still) seeing test failures with the require-effective-target patch 
applied? Or is the issue that this would not execute the tests as widely as 
might be possible? In principle I'm quite happy to relax the target patterns, 
although have been having issues with sparc (below)...

Re. "what the architectures have in common" is largely that these are the 
primary/secondary archs on which I've checked the test passes! I can now add 
mips and microblaze to this list, however I'm nervous of dropping the target 
entirely given the very large number of target architectures gcc supports; and 
e.g. IA64 (in ILP32 mode) generates an ashiftrt:DI by 31 places, not 
ashiftrt:SI, which does not match the simplification criteria in combine.c.

> This should be something like 
> 
>   { target aarch64*-*-* i?86-*-* powerpc*-*-* sparc*-*-* x86_64-*-* }
> 
> E.g. sparc-sun-solaris2.11 with -m64 is lp64, but would be excluded by
> your target list.  Keep the list sorted alphabetically and best add an
> explanation so others know what those targets have in common.

So I've built a stage-1 compiler with --target=sparc-sun-solaris2.11, and I find

   * without -m64, my "dg-require-effective-target ilp32" causes the 32-bit test 
to execute, and pass; "dg-require-effective-target lp64" prevents execution of 
the 64-bit test (which would fail) - so all as expected and desired.

   * with -lp64, behaviour is as previous (this is probably expected)

   * with -m64, "dg-require-effective-target ilp32" still causes the test to 
execute (but it fails, as the RTL now has an ashiftrt:DI by 31 places, which 
doesn't meet the simplification criteria in combine.c - this is pretty much as 
expected). "dg-require-effective-target lp64" stops the 64-bit test from 
executing however (despite that it would now pass).

Can you clarify what I should be doing on sparc, therefore?

Thanks for your help!

Alan

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

[Rainer Orth]

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

Alan Lawrence <alan.lawrence@arm.com> writes:

> Rainer Orth wrote:
>>> However, as a quick first step, does adding the ilp32 / lp64 (and keeping
>>> the architectures list for now) solve the immediate problem? Patch
>>> attached, OK for trunk?
>>
>> No, as I said this is wrong for biarch targets like sparc and i386.
>
> When you say no this does not solve the immediate problem, are you saying
> that you are (still) seeing test failures with the require-effective-target
> patch applied? Or is the issue that this would not execute the tests as

I didn't try that patch yet, but the target part is wrong, as I tried to
explain.  Consider the sparc case: 

* if you configure for sparc-sun-solaris2.11, you default to -m32
  (i.e. ilp32), while -m64 is lp64

* if you configure for sparcv9-sun-solaris2.11 instead, you default to
  -m64 (lp64), but get ilp32 with -m32

So, irrespective of the sparc vs. sparc64 (which is wrong, btw., the
canonical form for 64-bit-default sparc is sparcv9) forms, you can get
ilp32 and lp64 with both.

Similar issues hold for i?86 vs. x86_64 and probably other biarch
targets like powerpc vs. powerpc64, so you need to use the most generic
forms of the target names in you target lists.

> widely as might be possible? In principle I'm quite happy to relax the
> target patterns, although have been having issues with sparc (below)...
>
> Re. "what the architectures have in common" is largely that these are the
> primary/secondary archs on which I've checked the test passes! I can now
> add mips and microblaze to this list, however I'm nervous of dropping the
> target entirely given the very large number of target architectures gcc
> supports; and e.g. IA64 (in ILP32 mode) generates an ashiftrt:DI by 31
> places, not ashiftrt:SI, which does not match the simplification criteria
> in combine.c.

As I stated before, such target lists without any explanation are bound
to confuse future readers/testers: at the very least, add comments
explaining what those lists have in common.  OTOH, at this stage it
might be best to just drop the target list for now, learn which targets
pass and fail the tests, and then reintroduce them or, better yet, add
an effective-target keyword which matches them.  Otherwise, you'll never
get test coverage beyond your current list.

>> This should be something like 
>>
>>   { target aarch64*-*-* i?86-*-* powerpc*-*-* sparc*-*-* x86_64-*-* }
>>
>> E.g. sparc-sun-solaris2.11 with -m64 is lp64, but would be excluded by
>> your target list.  Keep the list sorted alphabetically and best add an
>> explanation so others know what those targets have in common.
>
> So I've built a stage-1 compiler with --target=sparc-sun-solaris2.11, and I find
>
>   * without -m64, my "dg-require-effective-target ilp32" causes the 32-bit
> test to execute, and pass; "dg-require-effective-target lp64" prevents
> execution of the 64-bit test (which would fail) - so all as expected and
> desired.
>
>   * with -lp64, behaviour is as previous (this is probably expected)

Huh?  What's -lp64?

>   * with -m64, "dg-require-effective-target ilp32" still causes the test to
> execute (but it fails, as the RTL now has an ashiftrt:DI by 31 places,
> which doesn't meet the simplification criteria in combine.c - this is
> pretty much as expected). "dg-require-effective-target lp64" stops the
> 64-bit test from executing however (despite that it would now pass).
>
> Can you clarify what I should be doing on sparc, therefore?

It's not only about sparc, but about all biarch targets.  The following
patch (which only includes the parts strictly necessary to avoid the
failures, nothing else I suggested above) works for me on
sparc-sun-solaris2.11 (-m32 and -m64), x86_64-unknown-linux-gnu (-m64
and -m32), and i686-unknown-linux-gnu (-m32 and -m64): the first test is
run for 64-bit only, while the second one only for 32-bit:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: ashift.patch --]
[-- Type: text/x-patch, Size: 1014 bytes --]

diff --git a/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c b/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
--- a/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
+++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
@@ -1,4 +1,5 @@
-/* { dg-do compile {target sparc64*-*-* aarch64*-*-* x86_64-*-* powerpc64*-*-*} } */
+/* { dg-do compile { target aarch64*-*-* i?86-*-* powerpc*-*-* sparc*-*-* x86_64-*-* } } */
+/* { dg-require-effective-target lp64 } */
 /* { dg-options "-O2 -fdump-rtl-combine-all" } */
 
 typedef long long int int64_t;
diff --git a/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c b/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
--- a/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
+++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
@@ -1,4 +1,5 @@
-/* { dg-do compile {target arm*-*-* i?86-*-* powerpc-*-* sparc-*-*} } */
+/* { dg-do compile { target arm*-*-* i?86-*-* powerpc*-*-* sparc*-*-* x86_64-*-* } } */
+/* { dg-require-effective-target ilp32 } */
 /* { dg-options "-O2 -fdump-rtl-combine-all" } */
 
 typedef long int32_t;

[-- Attachment #3: Type: text/plain, Size: 152 bytes --]


	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

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

[Alan Lawrence]

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

Well, I'd be happy with that. Curiously, that patch is identical to what I have 
here...and that's not what I got having built gcc with 
--target=sparc-sun-solaris2.11, but on further investigation it looks like the 
require-effective-target-ilp32/64 is not working correctly on my setup.

FWIW I've also tried MIPS (both 32- and 64-bit tests appear to work), IA64 
(64-bit test fine, not sure if ilp32 is supported), microblaze (32-bit), and 
AArch64 with -mabi=ilp32. Hence, I attach an alternative/possible patch which 
adds these platforms too.

However, since we have talked about removing the target selector altogether: the 
only arch I've found so far which has failed, was Alpha, and widening the regex 
to match "le" as well as "ge" then passes on Alpha too. To be honest I expect 
that if we were to go that route, we would find a deluge of smaller platforms on 
which the test fails; but if as testsuite maintainer you think that's 
appropriate - certainly I'd be willing to try that i.e. to exclude them as they 
turn up. A second alternative patch also attached. (FWIW I'll be away and unable 
to commit anything before Monday.)

More generally: really the test wants to be a unit test on combine_simplify_rtx, 
independent of front-end, expand, platform-specific insns, etc., but since we 
can't do that - whilst adding more platforms generally seems good, it is maybe 
not essential, and may increase fragility.

(In answer to your point about adding an effective-target in target-supports.exp 
- yes, could do that, but it's difficult to come up with a good characterization 
of what the criteria is, and I don't see it'd generalize to any other tests at 
all :(....)

--Alan

Rainer Orth wrote:
> Alan Lawrence <alan.lawrence@arm.com> writes:
> 
>> Rainer Orth wrote:
>>>> However, as a quick first step, does adding the ilp32 / lp64 (and keeping
>>>> the architectures list for now) solve the immediate problem? Patch
>>>> attached, OK for trunk?
>>> No, as I said this is wrong for biarch targets like sparc and i386.
>> When you say no this does not solve the immediate problem, are you saying
>> that you are (still) seeing test failures with the require-effective-target
>> patch applied? Or is the issue that this would not execute the tests as
> 
> I didn't try that patch yet, but the target part is wrong, as I tried to
> explain.  Consider the sparc case: 
> 
> * if you configure for sparc-sun-solaris2.11, you default to -m32
>   (i.e. ilp32), while -m64 is lp64
> 
> * if you configure for sparcv9-sun-solaris2.11 instead, you default to
>   -m64 (lp64), but get ilp32 with -m32
> 
> So, irrespective of the sparc vs. sparc64 (which is wrong, btw., the
> canonical form for 64-bit-default sparc is sparcv9) forms, you can get
> ilp32 and lp64 with both.
> 
> Similar issues hold for i?86 vs. x86_64 and probably other biarch
> targets like powerpc vs. powerpc64, so you need to use the most generic
> forms of the target names in you target lists.
> 
>> widely as might be possible? In principle I'm quite happy to relax the
>> target patterns, although have been having issues with sparc (below)...
>>
>> Re. "what the architectures have in common" is largely that these are the
>> primary/secondary archs on which I've checked the test passes! I can now
>> add mips and microblaze to this list, however I'm nervous of dropping the
>> target entirely given the very large number of target architectures gcc
>> supports; and e.g. IA64 (in ILP32 mode) generates an ashiftrt:DI by 31
>> places, not ashiftrt:SI, which does not match the simplification criteria
>> in combine.c.
> 
> As I stated before, such target lists without any explanation are bound
> to confuse future readers/testers: at the very least, add comments
> explaining what those lists have in common.  OTOH, at this stage it
> might be best to just drop the target list for now, learn which targets
> pass and fail the tests, and then reintroduce them or, better yet, add
> an effective-target keyword which matches them.  Otherwise, you'll never
> get test coverage beyond your current list.
> 
>>> This should be something like 
>>>
>>>   { target aarch64*-*-* i?86-*-* powerpc*-*-* sparc*-*-* x86_64-*-* }
>>>
>>> E.g. sparc-sun-solaris2.11 with -m64 is lp64, but would be excluded by
>>> your target list.  Keep the list sorted alphabetically and best add an
>>> explanation so others know what those targets have in common.
>> So I've built a stage-1 compiler with --target=sparc-sun-solaris2.11, and I find
>>
>>   * without -m64, my "dg-require-effective-target ilp32" causes the 32-bit
>> test to execute, and pass; "dg-require-effective-target lp64" prevents
>> execution of the 64-bit test (which would fail) - so all as expected and
>> desired.
>>
>>   * with -lp64, behaviour is as previous (this is probably expected)
> 
> Huh?  What's -lp64?
> 
>>   * with -m64, "dg-require-effective-target ilp32" still causes the test to
>> execute (but it fails, as the RTL now has an ashiftrt:DI by 31 places,
>> which doesn't meet the simplification criteria in combine.c - this is
>> pretty much as expected). "dg-require-effective-target lp64" stops the
>> 64-bit test from executing however (despite that it would now pass).
>>
>> Can you clarify what I should be doing on sparc, therefore?
> 
> It's not only about sparc, but about all biarch targets.  The following
> patch (which only includes the parts strictly necessary to avoid the
> failures, nothing else I suggested above) works for me on
> sparc-sun-solaris2.11 (-m32 and -m64), x86_64-unknown-linux-gnu (-m64
> and -m32), and i686-unknown-linux-gnu (-m32 and -m64): the first test is
> run for 64-bit only, while the second one only for 32-bit:

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

diff --git a/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c b/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
index 90e64fd10dc358f10ad03a90041605bc3ccb7011..6f92c253ce95ce16bdfd7f943030b77583bb7570 100644
--- a/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
+++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
@@ -1,4 +1,7 @@
-/* { dg-do compile {target sparc64*-*-* aarch64*-*-* x86_64-*-* powerpc64*-*-*} } */
+/* Target architectures which have been found to produce the expected RTL
+   (neg:DI (ge:DI ...)) when compiling for ILP32.  */
+/* { dg-do compile {target aarch64*-*-* ia64-*-* i?86-*-* mips*-*-* powerpc*-*-* sparc*-*-* x86_64-*-*} } */
+/* { dg-require-effective-target lp64 } */
 /* { dg-options "-O2 -fdump-rtl-combine-all" } */
 
 typedef long long int int64_t;
diff --git a/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c b/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
index fd6827caed230ea5dd2d6ec4431b11bf826531ea..c921d62b70681ca1cd0c51ab17c15d6b6c74930d 100644
--- a/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
+++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
@@ -1,4 +1,7 @@
-/* { dg-do compile {target arm*-*-* i?86-*-* powerpc-*-* sparc-*-*} } */
+/* Target architectures where RTL has been found to produce the expected
+   (neg:SI (ge:SI ...)) when compiling for ILP32.  */
+/* { dg-do compile {target aarch64*-*-* arm*-*-* i?86-*-* microblaze-*-* mips*-*-* powerpc*-*-* sparc*-*-* x86_64-*-*} } */
+/* { dg-require-effective-target ilp32} */
 /* { dg-options "-O2 -fdump-rtl-combine-all" } */
 
 typedef long int32_t;

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

diff --git a/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c b/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
index 90e64fd10dc358f10ad03a90041605bc3ccb7011..6f92c253ce95ce16bdfd7f943030b77583bb7570 100644
--- a/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
+++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
@@ -1,4 +1,7 @@
-/* { dg-do compile {target sparc64*-*-* aarch64*-*-* x86_64-*-* powerpc64*-*-*} } */
+/* Target architectures which have been found to produce the expected RTL
+   (neg:DI (ge:DI ...)) when compiling for ILP32.  */
+/* { dg-do compile {target aarch64*-*-* ia64-*-* i?86-*-* mips*-*-* powerpc*-*-* sparc*-*-* x86_64-*-*} } */
+/* { dg-require-effective-target lp64 } */
 /* { dg-options "-O2 -fdump-rtl-combine-all" } */
 
 typedef long long int int64_t;
diff --git a/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c b/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
index fd6827caed230ea5dd2d6ec4431b11bf826531ea..c921d62b70681ca1cd0c51ab17c15d6b6c74930d 100644
--- a/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
+++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
@@ -1,4 +1,7 @@
-/* { dg-do compile {target arm*-*-* i?86-*-* powerpc-*-* sparc-*-*} } */
+/* Target architectures where RTL has been found to produce the expected
+   (neg:SI (ge:SI ...)) when compiling for ILP32.  */
+/* { dg-do compile {target aarch64*-*-* arm*-*-* i?86-*-* microblaze-*-* mips*-*-* powerpc*-*-* sparc*-*-* x86_64-*-*} } */
+/* { dg-require-effective-target ilp32} */
 /* { dg-options "-O2 -fdump-rtl-combine-all" } */
 
 typedef long int32_t;

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

[Rainer Orth]

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

Alan,

sorry for letting the ball drop on this one.

> Well, I'd be happy with that. Curiously, that patch is identical to what I
> have here...and that's not what I got having built gcc with
> --target=sparc-sun-solaris2.11, but on further investigation it looks like
> the require-effective-target-ilp32/64 is not working correctly on my setup.
>
> FWIW I've also tried MIPS (both 32- and 64-bit tests appear to work), IA64
> (64-bit test fine, not sure if ilp32 is supported), microblaze (32-bit),

No, there's no 32-bit IA-64.

> and AArch64 with -mabi=ilp32. Hence, I attach an alternative/possible patch
> which adds these platforms too.

Huh?  The two patches seem completely identical to me...

> However, since we have talked about removing the target selector
> altogether: the only arch I've found so far which has failed, was Alpha,
> and widening the regex to match "le" as well as "ge" then passes on Alpha
> too. To be honest I expect that if we were to go that route, we would find
> a deluge of smaller platforms on which the test fails; but if as testsuite
> maintainer you think that's appropriate - certainly I'd be willing to try
> that i.e. to exclude them as they turn up. A second alternative patch also
> attached. (FWIW I'll be away and unable to commit anything before Monday.)

I'm still not really comfortable with those target lists; they tend to
artificially exclude tests on targets where they are perfectly capable
of running.  At least with the comments added, it's better than before
with no explanation whatsoever.  Perhaps Mike can weigh in here?

> More generally: really the test wants to be a unit test on
> combine_simplify_rtx, independent of front-end, expand, platform-specific
> insns, etc., but since we can't do that - whilst adding more platforms
> generally seems good, it is maybe not essential, and may increase
> fragility.

Might be, though we'll never know until we try.

> (In answer to your point about adding an effective-target in
> target-supports.exp - yes, could do that, but it's difficult to come up
> with a good characterization of what the criteria is, and I don't see it'd
> generalize to any other tests at all :(....)

Agreed: if this list isn't going to be used beyond those two tests,
there's no point in adding some artifical effective-target keyword.

I'm attaching an updated version of the patch with a few minor changes
of mine, noted below.

	Rainer


> Rainer Orth wrote:
>> Alan Lawrence <alan.lawrence@arm.com> writes:
>>
>>> Rainer Orth wrote:
>>>>> However, as a quick first step, does adding the ilp32 / lp64 (and keeping
>>>>> the architectures list for now) solve the immediate problem? Patch
>>>>> attached, OK for trunk?
>>>> No, as I said this is wrong for biarch targets like sparc and i386.
>>> When you say no this does not solve the immediate problem, are you saying
>>> that you are (still) seeing test failures with the require-effective-target
>>> patch applied? Or is the issue that this would not execute the tests as
>>
>> I didn't try that patch yet, but the target part is wrong, as I tried to
>> explain.  Consider the sparc case: 
>>
>> * if you configure for sparc-sun-solaris2.11, you default to -m32
>>   (i.e. ilp32), while -m64 is lp64
>>
>> * if you configure for sparcv9-sun-solaris2.11 instead, you default to
>>   -m64 (lp64), but get ilp32 with -m32
>>
>> So, irrespective of the sparc vs. sparc64 (which is wrong, btw., the
>> canonical form for 64-bit-default sparc is sparcv9) forms, you can get
>> ilp32 and lp64 with both.
>>
>> Similar issues hold for i?86 vs. x86_64 and probably other biarch
>> targets like powerpc vs. powerpc64, so you need to use the most generic
>> forms of the target names in you target lists.
>>
>>> widely as might be possible? In principle I'm quite happy to relax the
>>> target patterns, although have been having issues with sparc (below)...
>>>
>>> Re. "what the architectures have in common" is largely that these are the
>>> primary/secondary archs on which I've checked the test passes! I can now
>>> add mips and microblaze to this list, however I'm nervous of dropping the
>>> target entirely given the very large number of target architectures gcc
>>> supports; and e.g. IA64 (in ILP32 mode) generates an ashiftrt:DI by 31
>>> places, not ashiftrt:SI, which does not match the simplification criteria
>>> in combine.c.
>>
>> As I stated before, such target lists without any explanation are bound
>> to confuse future readers/testers: at the very least, add comments
>> explaining what those lists have in common.  OTOH, at this stage it
>> might be best to just drop the target list for now, learn which targets
>> pass and fail the tests, and then reintroduce them or, better yet, add
>> an effective-target keyword which matches them.  Otherwise, you'll never
>> get test coverage beyond your current list.
>>
>>>> This should be something like 
>>>>
>>>>   { target aarch64*-*-* i?86-*-* powerpc*-*-* sparc*-*-* x86_64-*-* }
>>>>
>>>> E.g. sparc-sun-solaris2.11 with -m64 is lp64, but would be excluded by
>>>> your target list.  Keep the list sorted alphabetically and best add an
>>>> explanation so others know what those targets have in common.
>>> So I've built a stage-1 compiler with --target=sparc-sun-solaris2.11,
>>> and I find
>>>
>>>   * without -m64, my "dg-require-effective-target ilp32" causes the 32-bit
>>> test to execute, and pass; "dg-require-effective-target lp64" prevents
>>> execution of the 64-bit test (which would fail) - so all as expected and
>>> desired.
>>>
>>>   * with -lp64, behaviour is as previous (this is probably expected)
>>
>> Huh?  What's -lp64?
>>
>>>   * with -m64, "dg-require-effective-target ilp32" still causes the test to
>>> execute (but it fails, as the RTL now has an ashiftrt:DI by 31 places,
>>> which doesn't meet the simplification criteria in combine.c - this is
>>> pretty much as expected). "dg-require-effective-target lp64" stops the
>>> 64-bit test from executing however (despite that it would now pass).
>>>
>>> Can you clarify what I should be doing on sparc, therefore?
>>
>> It's not only about sparc, but about all biarch targets.  The following
>> patch (which only includes the parts strictly necessary to avoid the
>> failures, nothing else I suggested above) works for me on
>> sparc-sun-solaris2.11 (-m32 and -m64), x86_64-unknown-linux-gnu (-m64
>> and -m32), and i686-unknown-linux-gnu (-m32 and -m64): the first test is
>> run for 64-bit only, while the second one only for 32-bit:
> diff --git a/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c b/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
> index 90e64fd10dc358f10ad03a90041605bc3ccb7011..6f92c253ce95ce16bdfd7f943030b77583bb7570 100644
> --- a/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
> +++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
> @@ -1,4 +1,7 @@
> -/* { dg-do compile {target sparc64*-*-* aarch64*-*-* x86_64-*-* powerpc64*-*-*} } */
> +/* Target architectures which have been found to produce the expected RTL
> +   (neg:DI (ge:DI ...)) when compiling for ILP32.  */

LP64 here.

> +/* { dg-do compile {target aarch64*-*-* ia64-*-* i?86-*-* mips*-*-* powerpc*-*-* sparc*-*-* x86_64-*-*} } */

Please have whitespace here: "{ target" and here: "x86_64-*-* }".

> +/* { dg-require-effective-target lp64 } */
>  /* { dg-options "-O2 -fdump-rtl-combine-all" } */
>  
>  typedef long long int int64_t;
> diff --git a/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c b/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
> index fd6827caed230ea5dd2d6ec4431b11bf826531ea..c921d62b70681ca1cd0c51ab17c15d6b6c74930d 100644
> --- a/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
> +++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
> @@ -1,4 +1,7 @@
> -/* { dg-do compile {target arm*-*-* i?86-*-* powerpc-*-* sparc-*-*} } */
> +/* Target architectures where RTL has been found to produce the expected
> +   (neg:SI (ge:SI ...)) when compiling for ILP32.  */
> +/* { dg-do compile {target aarch64*-*-* arm*-*-* i?86-*-* microblaze-*-* mips*-*-* powerpc*-*-* sparc*-*-* x86_64-*-*} } */

Same whitespace issue...

> +/* { dg-require-effective-target ilp32} */
>  /* { dg-options "-O2 -fdump-rtl-combine-all" } */
>  
>  typedef long int32_t;
> diff --git a/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c b/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
> index 90e64fd10dc358f10ad03a90041605bc3ccb7011..6f92c253ce95ce16bdfd7f943030b77583bb7570 100644
> --- a/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
> +++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
> @@ -1,4 +1,7 @@
> -/* { dg-do compile {target sparc64*-*-* aarch64*-*-* x86_64-*-* powerpc64*-*-*} } */
> +/* Target architectures which have been found to produce the expected RTL
> +   (neg:DI (ge:DI ...)) when compiling for ILP32.  */
> +/* { dg-do compile {target aarch64*-*-* ia64-*-* i?86-*-* mips*-*-* powerpc*-*-* sparc*-*-* x86_64-*-*} } */
> +/* { dg-require-effective-target lp64 } */
>  /* { dg-options "-O2 -fdump-rtl-combine-all" } */
>  
>  typedef long long int int64_t;
> diff --git a/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c b/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
> index fd6827caed230ea5dd2d6ec4431b11bf826531ea..c921d62b70681ca1cd0c51ab17c15d6b6c74930d 100644
> --- a/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
> +++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
> @@ -1,4 +1,7 @@
> -/* { dg-do compile {target arm*-*-* i?86-*-* powerpc-*-* sparc-*-*} } */
> +/* Target architectures where RTL has been found to produce the expected
> +   (neg:SI (ge:SI ...)) when compiling for ILP32.  */
> +/* { dg-do compile {target aarch64*-*-* arm*-*-* i?86-*-* microblaze-*-* mips*-*-* powerpc*-*-* sparc*-*-* x86_64-*-*} } */
> +/* { dg-require-effective-target ilp32} */
>  /* { dg-options "-O2 -fdump-rtl-combine-all" } */
>  
>  typedef long int32_t;


2015-01-29  Alan Lawrence  <alan.lawrence@arm.com>
	    Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>

	gcc/testsuite:
	* gcc.dg/combine_ashiftrt_1.c: Sort, complete and explain target
	list, allow for multilibed targets.
	* gcc.dg/combine_ashiftrt_2.c: Likewise.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: testsuite-combine_ashiftrt.patch --]
[-- Type: text/x-patch, Size: 1432 bytes --]

# HG changeset patch
# Parent aedb8ec64c1c4c74e30210d024845e2b0b2dc1eb
Properly check for 32 vs. 64-bit in gcc.dg/combine_ashiftrt_[12].c

diff --git a/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c b/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
--- a/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
+++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
@@ -1,4 +1,6 @@
-/* { dg-do compile {target sparc64*-*-* aarch64*-*-* i?86-*-* x86_64-*-* powerpc64*-*-*} } */
+/* Target architectures which have been found to produce the expected RTL
+   (neg:DI (ge:DI ...)) when compiling for LP64.  */
+/* { dg-do compile { target aarch64*-*-* arm*-*-* i?86-*-* ia64-*-* powerpc*-*-* sparc*-*-* x86_64-*-* } } */
 /* { dg-require-effective-target lp64 } */
 /* { dg-options "-O2 -fdump-rtl-combine-all" } */
 
diff --git a/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c b/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
--- a/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
+++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
@@ -1,4 +1,6 @@
-/* { dg-do compile {target arm*-*-* i?86-*-* x86_64-*-* powerpc-*-* sparc-*-*} } */
+/* Target architectures where RTL has been found to produce the expected
+   (neg:SI (ge:SI ...)) when compiling for ILP32.  */
+/* { dg-do compile { target aarch64*-*-* arm*-*-* i?86-*-* microblaze-*-* mips*-*-* powerpc*-*-* sparc*-*-* x86_64-*-* } } */
 /* { dg-require-effective-target ilp32 } */
 /* { dg-options "-O2 -fdump-rtl-combine-all" } */
 

[-- Attachment #3: Type: text/plain, Size: 143 bytes --]


-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

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

[Alan Lawrence]

Rainer Orth wrote:
>
> I'm still not really comfortable with those target lists; they tend to
> artificially exclude tests on targets where they are perfectly capable
> of running.  At least with the comments added, it's better than before
> with no explanation whatsoever.  Perhaps Mike can weigh in here?

Well, it's been awhile, but on further reflection - my feeling is that we should 
be dropping the target lists here too. Maybe we end up introducing a dg-skip-if 
that grows over time, but it'd have to grow quite a bit to reach the size of the 
dg-do target we'd otherwise have...

However I am a bit wary about dropping the dg-do target constraint just as we 
are nearing a release! So if we were to keep the whitelist approach, your patch 
looks good to me, and I'd be happy if that were committed.

Cheers, Alan

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

[Rainer Orth]

Hi Alan,

>> I'm still not really comfortable with those target lists; they tend to
>> artificially exclude tests on targets where they are perfectly capable
>> of running.  At least with the comments added, it's better than before
>> with no explanation whatsoever.  Perhaps Mike can weigh in here?
>
> Well, it's been awhile, but on further reflection - my feeling is that we
> should be dropping the target lists here too. Maybe we end up introducing a
> dg-skip-if that grows over time, but it'd have to grow quite a bit to reach
> the size of the dg-do target we'd otherwise have...

It's not even necessary to use dg-skip if the scan-rtl-dump fails.  You
can just add an xfail there, which has the advantage that you do notice
if the test starts to pass e.g. due to changes in a target.

> However I am a bit wary about dropping the dg-do target constraint just as
> we are nearing a release! So if we were to keep the whitelist approach,
> your patch looks good to me, and I'd be happy if that were committed.

Let's give others a day or two to comment: if nobody is in favour of the
more agressive approach, I'll commit my patch.

Thanks.
        Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

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

[Jeff Law]

On 09/18/14 03:35, Alan Lawrence wrote:
> Moreover, I think we both agree that if result_mode==shift_mode, the
> transformation is correct. "Just" putting that check in, achieves
> what I'm trying for here, so I'd be happy to go with the attached
> patch and call it a day. However, I'm a little concerned about the
> other cases - i.e. where shift_mode is wider than result_mode.
Let's go ahead and get the attached patch installed.  I'm pretty sure 
it's correct and I know you want to see something move forward here.  We 
can iterate further if we want.

>
> If I understand correctly (and I'm not sure about that, let's see how
> far I get), this means we'll perform the shift in (say) DImode, when
> we're only really concerned about the lower (say) 32-bits (for an
> originally-SImode shift).
That's the class of cases I'm concerned about.


  try_widen_shift_mode will in this case
> check that the result of the operation *inside* the shift (in our
> case an XOR) has 33 sign bit copies (via num_sign_bit_copies), i.e.
> that its *top* 32-bits are all equal to the original SImode sign bit.
> <count> of these bits may be fed into the top of the desired SImode
> result by the DImode shift. Right so far?
Correct.

>
> AFAICT, num_sign_bit_copies for an XOR, conservatively returns the
> minimum of the num_sign_bit_copies of its two operands. I'm not sure
> whether this is behaviour we should rely on in its callers, or for
> the sake of abstraction we should treat num_sign_bit_copies as a
> black box (which does what it says on the, erm, tin).
Treat it as a black box.  It returns the number of known sign bit 
copies.  There may be more, but never less.


>
> If the former, doesn't having num_sign_bit_copies >= the difference
> in size between result_mode and shift_mode, of both operands to the
> XOR, guarantee safety of the commutation (whether the constant is
> positive or negative)? We could perform the shift (in the larger
> mode) on both of the XOR operands safely, then XOR together their
> lower parts.
I had convinced myself that when we flip the sign bit via the XOR and 
commute the XOR out that we invalidate the assumptions made when 
widening.  But I'm not so sure anymore.  Damn I hate changes made 
without suitable tests :(

I almost convinced myself the problem is in the adjustment of C2 in the 
widened case, but that's not a problem either.  At least not on paper.

>
> If, however, we want to play safe and ensure that we deal safely with
>  any XOR whose top (mode size difference + 1) bits were the same,
> then I think the restriction that the XOR constant is positive is
> neither necessary nor sufficient; rather (mirroring
> try_widen_shift_mode) I think we need that num_sign_bit_copies of the
> constant in shift_mode, is more than the size difference between
> result_mode and shift_mode.
But isn't that the same?  Isn't the only case where it isn't the same 
when the constant has bits set that are outside the mode of the other 
operand?

Hmm, what about (xor:QI A -1)?  In that case -1 will be represented with 
bits outside the precision of QImode.

>
> Hmmm. I might try that patch at some point, I think it is the right
> check to make. (Meta-comment: this would be *so*much* easier if we
> could write unit tests more easily!) In the meantime I'd be happy to
> settle for the attached...
No argument on the unit testing comment.  It's a major failing in the 
design of GCC that we can't easily build a unit testing framework.

Jeff

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

[Alan Lawrence]

Ok thanks Jeff. In that case I think I should draw this to the attention of the 
AArch64 maintainers to check the testsuite updates are OK before I commit...?

Methinks it may be possible to get further, or at least increase our confidence, 
if I "mock" out try_widen_shift_mode, and/or try injecting some dubious RTL from 
a builtin, although this'll only give a momentary snapshot of behaviour. I may 
or may not have time to look into this though ;)...

Cheers, Alan

Jeff Law wrote:
> On 09/18/14 03:35, Alan Lawrence wrote:
>> Moreover, I think we both agree that if result_mode==shift_mode, the
>> transformation is correct. "Just" putting that check in, achieves
>> what I'm trying for here, so I'd be happy to go with the attached
>> patch and call it a day. However, I'm a little concerned about the
>> other cases - i.e. where shift_mode is wider than result_mode.
> Let's go ahead and get the attached patch installed.  I'm pretty sure 
> it's correct and I know you want to see something move forward here.  We 
> can iterate further if we want.
> 
>> If I understand correctly (and I'm not sure about that, let's see how
>> far I get), this means we'll perform the shift in (say) DImode, when
>> we're only really concerned about the lower (say) 32-bits (for an
>> originally-SImode shift).
> That's the class of cases I'm concerned about.
> 
> 
>   try_widen_shift_mode will in this case
>> check that the result of the operation *inside* the shift (in our
>> case an XOR) has 33 sign bit copies (via num_sign_bit_copies), i.e.
>> that its *top* 32-bits are all equal to the original SImode sign bit.
>> <count> of these bits may be fed into the top of the desired SImode
>> result by the DImode shift. Right so far?
> Correct.
> 
>> AFAICT, num_sign_bit_copies for an XOR, conservatively returns the
>> minimum of the num_sign_bit_copies of its two operands. I'm not sure
>> whether this is behaviour we should rely on in its callers, or for
>> the sake of abstraction we should treat num_sign_bit_copies as a
>> black box (which does what it says on the, erm, tin).
> Treat it as a black box.  It returns the number of known sign bit 
> copies.  There may be more, but never less.
> 
> 
>> If the former, doesn't having num_sign_bit_copies >= the difference
>> in size between result_mode and shift_mode, of both operands to the
>> XOR, guarantee safety of the commutation (whether the constant is
>> positive or negative)? We could perform the shift (in the larger
>> mode) on both of the XOR operands safely, then XOR together their
>> lower parts.
> I had convinced myself that when we flip the sign bit via the XOR and 
> commute the XOR out that we invalidate the assumptions made when 
> widening.  But I'm not so sure anymore.  Damn I hate changes made 
> without suitable tests :(
> 
> I almost convinced myself the problem is in the adjustment of C2 in the 
> widened case, but that's not a problem either.  At least not on paper.
> 
>> If, however, we want to play safe and ensure that we deal safely with
>>  any XOR whose top (mode size difference + 1) bits were the same,
>> then I think the restriction that the XOR constant is positive is
>> neither necessary nor sufficient; rather (mirroring
>> try_widen_shift_mode) I think we need that num_sign_bit_copies of the
>> constant in shift_mode, is more than the size difference between
>> result_mode and shift_mode.
> But isn't that the same?  Isn't the only case where it isn't the same 
> when the constant has bits set that are outside the mode of the other 
> operand?
> 
> Hmm, what about (xor:QI A -1)?  In that case -1 will be represented with 
> bits outside the precision of QImode.
> 
>> Hmmm. I might try that patch at some point, I think it is the right
>> check to make. (Meta-comment: this would be *so*much* easier if we
>> could write unit tests more easily!) In the meantime I'd be happy to
>> settle for the attached...
> No argument on the unit testing comment.  It's a major failing in the 
> design of GCC that we can't easily build a unit testing framework.
> 
> Jeff
> 

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

[Jeff Law]

On 09/22/14 05:16, Alan Lawrence wrote:
> Ok thanks Jeff. In that case I think I should draw this to the attention
> of the AArch64 maintainers to check the testsuite updates are OK before
> I commit...?
Can't hurt.

>
> Methinks it may be possible to get further, or at least increase our
> confidence, if I "mock" out try_widen_shift_mode, and/or try injecting
> some dubious RTL from a builtin, although this'll only give a momentary
> snapshot of behaviour. I may or may not have time to look into this
> though ;)...
Yea, it's something I'd pondered as well, though I tend to inject the 
RTL I want directly in the debugger.  The downside is doing so doesn't 
ensure various tables are updated properly, and I vaguely recall a 
per-pseudo table for the combiner's nonzero_bits, signbit_copies and 
friends.


jeff

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

[Marcus Shawcroft]

On 22 September 2014 12:16, Alan Lawrence <alan.lawrence@arm.com> wrote:
> Ok thanks Jeff. In that case I think I should draw this to the attention of
> the AArch64 maintainers to check the testsuite updates are OK before I
> commit...?

OK with me.

Cheers
/Marcus