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

[Richard Earnshaw <Richard dot Earnshaw at foss dot arm dot com>]

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