[AArch64/GCC] PR64304, miscompilation with -mgeneral-regs-only

as discussed here https://gcc.gnu.org/ml/gcc-patches/2015-01/msg00563.html

the problem is aarch64 hardware only support left shift truncation for SI/DI,
while SHIFT_COUNT_TRUNCATED is enabled for all mode including QI/HI, which is
inconsistent with hardware feature.

there are two patterns defined for ashift:QI/HI, one for shift amount in register,
one for shift amount as constant.

this patch only remove the pattern for shift amount in register which cause the trouble.

no regression on bare metal test.
bootstrap OK.

ok for trunk?


2015-01-15 Jiong. Wang (jiong.wang@arm.com)
gcc/
PR64304
   * config/aarch64/aarch64.md (define_insn "*ashl<mode>3_insn"): Deleted.
   (ashl<mode>3): Don't expand if operands[2] is not constant.

gcc/testsuite/
   * gcc.target/aarch64/pr64304.c: New testcase.

On 14 January 2015 at 15:31, Jiong Wang <jiong.wang@arm.com> wrote:

> 2015-01-15 Jiong. Wang (jiong.wang@arm.com)
> gcc/
> PR64304
>   * config/aarch64/aarch64.md (define_insn "*ashl<mode>3_insn"): Deleted.
>   (ashl<mode>3): Don't expand if operands[2] is not constant.
>
> gcc/testsuite/
>   * gcc.target/aarch64/pr64304.c: New testcase.

@@ -3091,6 +3091,8 @@
     DONE;
           }
       }
+    else
+      DONE;
   }
 )

Did you mean FAIL ? In the current form this says the expander matched
by no RTL is required to implement the shift.
/Marcus

On 16/01/15 10:50, Marcus Shawcroft wrote:
> On 14 January 2015 at 15:31, Jiong Wang <jiong.wang@arm.com> wrote:
>
>> 2015-01-15 Jiong. Wang (jiong.wang@arm.com)
>> gcc/
>> PR64304
>>    * config/aarch64/aarch64.md (define_insn "*ashl<mode>3_insn"): Deleted.
>>    (ashl<mode>3): Don't expand if operands[2] is not constant.
>>
>> gcc/testsuite/
>>    * gcc.target/aarch64/pr64304.c: New testcase.
> @@ -3091,6 +3091,8 @@
>       DONE;
>             }
>         }
> +    else
> +      DONE;
>     }
>   )
>
> Did you mean FAIL ?

exactly, thanks, we should use FAIL although DONE and FAIL work the same in this scenario.

I checked their definition, FAIL always return the initial value of _val which is NULL,
while DONE stop and return generated insns which for this pattern happen to be NULL also. that
explain why it's not exposed by testing.


#define FAIL return (end_sequence (), _val)
#define DONE return (_val = get_insns (), end_sequence (), _val)

> In the current form this says the expander matched
> by no RTL is required to implement the shift.
> /Marcus
>

On 16 January 2015 at 11:17, Jiong Wang <jiong.wang@arm.com> wrote:

> exactly, thanks, we should use FAIL although DONE and FAIL work the same in
> this scenario.
>
> I checked their definition, FAIL always return the initial value of _val
> which is NULL,
> while DONE stop and return generated insns which for this pattern happen to
> be NULL also. that
> explain why it's not exposed by testing.
>
>
> #define FAIL return (end_sequence (), _val)
> #define DONE return (_val = get_insns (), end_sequence (), _val)

It seems rather odd to me that DONE behaves in this way.  Your patch
is OK with DONE switched to FAIL.  /Marcus

On 14/01/15 15:31, Jiong Wang wrote:
> as discussed here https://gcc.gnu.org/ml/gcc-patches/2015-01/msg00563.html
> 
> the problem is aarch64 hardware only support left shift truncation for SI/DI,
> while SHIFT_COUNT_TRUNCATED is enabled for all mode including QI/HI, which is
> inconsistent with hardware feature.
> 
> there are two patterns defined for ashift:QI/HI, one for shift amount in register,
> one for shift amount as constant.
> 
> this patch only remove the pattern for shift amount in register which cause the trouble.
> 
> no regression on bare metal test.
> bootstrap OK.
> 
> ok for trunk?
> 

Rather late for a follow-up, but I wonder if it might be better for the
AArch64 port to use TARGET_SHIFT_TRUNCATION_MASK rather than defining
SHIFT_COUNT_TRUNCATED.  This seems to better explain the semantics of
the operation.

R.

> 
> 2015-01-15 Jiong. Wang (jiong.wang@arm.com)
> gcc/
> PR64304
>    * config/aarch64/aarch64.md (define_insn "*ashl<mode>3_insn"): Deleted.
>    (ashl<mode>3): Don't expand if operands[2] is not constant.
> 
> gcc/testsuite/
>    * gcc.target/aarch64/pr64304.c: New testcase.
> 
> 
> fix-md.patch
> 
> 
> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
> index 597ff8c..49d6690 100644
> --- a/gcc/config/aarch64/aarch64.md
> +++ b/gcc/config/aarch64/aarch64.md
> @@ -3091,6 +3091,8 @@
>  	    DONE;
>            }
>        }
> +    else
> +      DONE;
>    }
>  )
>  
> @@ -3315,15 +3317,6 @@
>    [(set_attr "type" "shift_reg")]
>  )
>  
> -(define_insn "*ashl<mode>3_insn"
> -  [(set (match_operand:SHORT 0 "register_operand" "=r")
> -	(ashift:SHORT (match_operand:SHORT 1 "register_operand" "r")
> -		      (match_operand:QI 2 "aarch64_reg_or_shift_imm_si" "rUss")))]
> -  ""
> -  "lsl\\t%<w>0, %<w>1, %<w>2"
> -  [(set_attr "type" "shift_reg")]
> -)
> -
>  (define_insn "*<optab><mode>3_insn"
>    [(set (match_operand:SHORT 0 "register_operand" "=r")
>  	(ASHIFT:SHORT (match_operand:SHORT 1 "register_operand" "r")
> diff --git a/gcc/testsuite/gcc.target/aarch64/pr64304.c b/gcc/testsuite/gcc.target/aarch64/pr64304.c
> new file mode 100644
> index 0000000..5423bb3
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/pr64304.c
> @@ -0,0 +1,18 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 --save-temps" } */
> +
> +unsigned char byte = 0;
> +
> +void
> +set_bit (unsigned int bit, unsigned char value)
> +{
> +  unsigned char mask = (unsigned char) (1 << (bit & 7));
> +
> +  if (! value)
> +    byte &= (unsigned char)~mask;
> +  else
> +    byte |= mask;
> +  /* { dg-final { scan-assembler "and\tw\[0-9\]+, w\[0-9\]+, 7" } } */
> +}
> +
> +/* { dg-final { cleanup-saved-temps } } */
>