On Tue, 26 Apr 2022, Jakub Jelinek wrote:
> Hi!
>
> The following testcase regressed on riscv due to the splitting of critical
> edges in the sink pass, similarly to x86_64 compared to GCC 11 we now swap
> the edges, whether true or false edge goes to an empty forwarded bb.
> From GIMPLE POV, those 2 forms are equivalent, but as can be seen here, for
> some ifcvt opts it matters one way or another.
Do we have evidence that one or the other form is "better"? There's
the possibility to change CFG cleanup to process the edges in a
defined {true,false} or {false,true} order rather than edge number order.
Likewise we could order fallthru before EH or abnormal edges here.
It would be a bit intrusive (and come at a cost) since currently we
just iterate over all BBs, seeing if they are forwarders while ordering
would require a different iteration scheme. But doing all this might
also make the CFG cleanup result more stable with respect to IL
representation changes.
> On this testcase, noce_try_store_flag_mask used to trigger and transformed
> if (pseudo2) pseudo1 = 0;
> into
> pseudo1 &= -(pseudo2 == 0);
> But with the swapped edges ifcvt actually sees
> if (!pseudo2) pseudo3 = pseudo1; else pseudo3 = 0;
> and noce_try_store_flag_mask punts. IMHO there is no reason why it
> should punt those, it is equivalent to
> pseudo3 = pseudo1 & -(pseudo2 == 0);
> and especially if the target has 3 operand AND, it shouldn't be any more
> costly (and even with 2 operand AND, it might very well happen that RA
> can make it happen without any extra moves).
>
> Initially I've just removed the rtx_equal_p calls from the conditions
> and didn't add anything there, but that broke aarch64 bootstrap and
> regressed some testcases on x86_64, where if_info->a or if_info->b could be
> some larger expression that we can't force into a register.
> Furthermore, the case where both if_info->a and if_info->b are constants is
> better handled by other ifcvt optimizations like noce_try_store_flag
> or noce_try_inverse_constants or noce_try_store_flag_constants.
> So, I've restricted it to just a REG (perhaps SUBREG of REG might be ok too)
> next to what has been handled previously.
>
> Bootstrapped/regtested on {x86_64,i686,armv7hl,aarch64,powerpc64le}-linux,
> and tested it on the testcase in a cross to riscv*-linux, ok for trunk?
OK.
Thanks,
Richard.
> 2022-04-26 Jakub Jelinek <ja...@redhat.com>
>
> PR rtl-optimization/105314
> * ifcvt.cc (noce_try_store_flag_mask): Don't require that the non-zero
> operand is equal to if_info->x, instead use the non-zero operand
> as one of the operands of AND with if_info->x as target.
>
> * gcc.target/riscv/pr105314.c: New test.
>
> --- gcc/ifcvt.cc.jj 2022-03-15 09:11:55.312988179 +0100
> +++ gcc/ifcvt.cc 2022-04-25 17:37:11.278924377 +0200
> @@ -1678,10 +1678,10 @@ noce_try_store_flag_mask (struct noce_if
> reversep = 0;
>
> if ((if_info->a == const0_rtx
> - && rtx_equal_p (if_info->b, if_info->x))
> + && (REG_P (if_info->b) || rtx_equal_p (if_info->b, if_info->x)))
> || ((reversep = (noce_reversed_cond_code (if_info) != UNKNOWN))
> && if_info->b == const0_rtx
> - && rtx_equal_p (if_info->a, if_info->x)))
> + && (REG_P (if_info->a) || rtx_equal_p (if_info->a, if_info->x))))
> {
> start_sequence ();
> target = noce_emit_store_flag (if_info,
> @@ -1689,7 +1689,7 @@ noce_try_store_flag_mask (struct noce_if
> reversep, -1);
> if (target)
> target = expand_simple_binop (GET_MODE (if_info->x), AND,
> - if_info->x,
> + reversep ? if_info->a : if_info->b,
> target, if_info->x, 0,
> OPTAB_WIDEN);
>
> --- gcc/testsuite/gcc.target/riscv/pr105314.c.jj 2022-04-25
> 17:41:00.958736306 +0200
> +++ gcc/testsuite/gcc.target/riscv/pr105314.c 2022-04-25 17:40:46.237940642
> +0200
> @@ -0,0 +1,12 @@
> +/* PR rtl-optimization/105314 */
> +/* { dg-do compile } *
> +/* { dg-options "-O2" } */
> +/* { dg-final { scan-assembler-not "\tbeq\t" } } */
> +
> +long
> +foo (long a, long b, long c)
> +{
> + if (c)
> + a = 0;
> + return a;
> +}
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)
