On 14/03/18 08:43, Jakub Jelinek wrote:
> Hi!
>
> The following testcase ICEs on aarch64-linux, because combiner matches the
> (insn 25 24 26 2 (set (reg:DI 114)
> (rotatert:DI (reg:DI 115 [ d ])
> (subreg:QI (neg:SI (and:SI (reg:SI 112)
> (const_int 65535 [0xffff]))) 0))) "pr84845.c":8 664
> {*aarch64_reg_di3_neg_mask2}
> (expr_list:REG_DEAD (reg:SI 112)
> (expr_list:REG_DEAD (reg:DI 115 [ d ])
> (nil))))
> pattern, but what we split it into doesn't recog, because there are just
> patterns that use the same mode for the shift/rotate as for AND and use
> proper
> (match_operand 3 "const_int_operand" "n"))])))]
> "(~INTVAL (operands[3]) & (GET_MODE_BITSIZE (<MODE>mode) - 1)) == 0"
> and then there is another pattern for the mixed DImode shift/rotate and
> SImode mask, but that one uses for the mask instead
> (match_operand 3 "aarch64_shift_imm_di" "Usd"))])))]
> "((~INTVAL (operands[3]) & (GET_MODE_BITSIZE (DImode)-1)) == 0)"
> which due to the predicate (and constraint) never matches.
>
> The following patch fixes that bug (uses "const_int_operand" "n" like in
> similar patterns), also fixes another bug in the
> *aarch64_reg_<mode>3_neg_mask2
> splitter, where if it isn't split before reload it would emit invalid
> negation (SImode negation from SImode input into DImode output).
>
> And the last change is that I find these patterns to be misnamed, the most
> important on these patterns is that they are shifts/rotates, but the names
> of the pattern don't indicate that at all; some other patterns around do
> indicate that with <optab>_ part, which the patch changes them to, and
> one pattern has that part in inconsistent spot to the others.
>
> Bootstrapped/regtested on aarch64-linux, ok for trunk?
OK.
R.
>
> 2018-03-14 Jakub Jelinek <ja...@redhat.com>
>
> PR target/84845
> * config/aarch64/aarch64.md (*aarch64_reg_<mode>3_neg_mask2): Rename
> to ...
> (*aarch64_<optab>_reg_<mode>3_neg_mask2): ... this. If pseudos can't
> be created, use lowpart_subreg of operands[0] rather than operands[0]
> itself.
> (*aarch64_reg_<mode>3_minus_mask): Rename to ...
> (*aarch64_ashl_reg_<mode>3_minus_mask): ... this.
> (*aarch64_<optab>_reg_di3_mask2): Use const_int_operand predicate
> and n constraint instead of aarch64_shift_imm_di and Usd.
> (*aarch64_reg_<optab>_minus<mode>3): Rename to ...
> (*aarch64_<optab>_reg_minus<mode>3): ... this.
>
> * gcc.c-torture/compile/pr84845.c: New test.
>
> --- gcc/config/aarch64/aarch64.md.jj 2018-03-13 00:38:26.000000000 +0100
> +++ gcc/config/aarch64/aarch64.md 2018-03-13 18:33:47.657719021 +0100
> @@ -4262,7 +4262,7 @@ (define_insn "*aarch64_<optab>_reg_<mode
> [(set_attr "type" "shift_reg")]
> )
>
> -(define_insn_and_split "*aarch64_reg_<mode>3_neg_mask2"
> +(define_insn_and_split "*aarch64_<optab>_reg_<mode>3_neg_mask2"
> [(set (match_operand:GPI 0 "register_operand" "=&r")
> (SHIFT:GPI
> (match_operand:GPI 1 "register_operand" "r")
> @@ -4275,7 +4275,7 @@ (define_insn_and_split "*aarch64_reg_<mo
> [(const_int 0)]
> {
> rtx tmp = (can_create_pseudo_p () ? gen_reg_rtx (SImode)
> - : operands[0]);
> + : lowpart_subreg (SImode, operands[0], <MODE>mode));
> emit_insn (gen_negsi2 (tmp, operands[2]));
>
> rtx and_op = gen_rtx_AND (SImode, tmp, operands[3]);
> @@ -4286,7 +4286,7 @@ (define_insn_and_split "*aarch64_reg_<mo
> }
> )
>
> -(define_insn_and_split "*aarch64_reg_<mode>3_minus_mask"
> +(define_insn_and_split "*aarch64_ashl_reg_<mode>3_minus_mask"
> [(set (match_operand:GPI 0 "register_operand" "=&r")
> (ashift:GPI
> (match_operand:GPI 1 "register_operand" "r")
> @@ -4320,8 +4320,8 @@ (define_insn "*aarch64_<optab>_reg_di3_m
> (match_operand:DI 1 "register_operand" "r")
> (match_operator 4 "subreg_lowpart_operator"
> [(and:SI (match_operand:SI 2 "register_operand" "r")
> - (match_operand 3 "aarch64_shift_imm_di" "Usd"))])))]
> - "((~INTVAL (operands[3]) & (GET_MODE_BITSIZE (DImode)-1)) == 0)"
> + (match_operand 3 "const_int_operand" "n"))])))]
> + "((~INTVAL (operands[3]) & (GET_MODE_BITSIZE (DImode) - 1)) == 0)"
> {
> rtx xop[3];
> xop[0] = operands[0];
> @@ -4333,7 +4333,7 @@ (define_insn "*aarch64_<optab>_reg_di3_m
> [(set_attr "type" "shift_reg")]
> )
>
> -(define_insn_and_split "*aarch64_reg_<optab>_minus<mode>3"
> +(define_insn_and_split "*aarch64_<optab>_reg_minus<mode>3"
> [(set (match_operand:GPI 0 "register_operand" "=&r")
> (ASHIFT:GPI
> (match_operand:GPI 1 "register_operand" "r")
> --- gcc/testsuite/gcc.c-torture/compile/pr84845.c.jj 2018-03-13
> 18:42:46.841004726 +0100
> +++ gcc/testsuite/gcc.c-torture/compile/pr84845.c 2018-03-13
> 18:42:16.491988632 +0100
> @@ -0,0 +1,12 @@
> +/* PR target/84845 */
> +
> +int a, b, c;
> +unsigned long d;
> +
> +void
> +foo (void)
> +{
> + b = -1;
> + b <<= c >= 0;
> + d = d << (63 & (short)-b) | d >> (63 & -(short)-b);
> +}
>
> Jakub
>
