Relax check against commuting XOR and ASHIFTRT in combine.c

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.

---------------------------------

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