Re: [PATCHv2, rs6000] Enable have_cbranchcc4 on rs6000

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi!

On Mon, Nov 21, 2022 at 02:18:39PM +0800, HAO CHEN GUI wrote:
> 在 2022/11/18 20:18, Segher Boessenkool 写道:
> > I don't think we should pretend we have any conditional jumps the
> > machine does not actually have, in cbranchcc4.  When would this ever be
> > useful?  cror;beq can be quite expensive, compared to the code it would
> > replace anyway.
> > 
> > If something generates those here (which then ICEs later), that is
> > wrong, fix *that*?  Is it ifcvt doing it?
> 
> "*cbranch_2insn" is a valid insn for rs6000. So it generates such insn
> at expand pass. The "prepare_cmp_insn" called by ifcvt just wants to verify
> that the following comparison rtx is valid.

*cbranch_2insn is not a machine insn.  It generates a cror and a branch
insn.  This makes no sense to have in a cbranchcc: those do a branch
based on an existing cr field, so based on the *output* of that cror.

If ifcvt requires differently, ifcvt needs fixing.

We want to use the output of the cror multiple times, not generate more
cror insns.

> (unlt (reg:CCFP 156)
>     (const_int 0 [0]))
> 
> It should be valid as it's extracted from an existing insn.

Why is that an argument?  The code is valid, sure, but that doesn't
mean we want to generate it all over the place.

> It hits ICE only
> when the comparison rtx can't pass the predicate check of "cbranchcc4". So
> "cbranchcc4" should include "extra_insn_branch_comparison_operator".
> 
> Then, ifcvt tries to call emit_conditional_move_1 to generates a condition
> move for FP mode. It definitely fails as there is no conditional move insn for
> FP mode in rs6000. The behavior of ifcvt is correct. It tries to do conversion
> but fails. It won't hit ICEs after cbranchcc4 is correctly defined.

I don't think the behaviour of ifcvt is correct here at all, no.  It
also does not consider the cost of the code as far as I can see?  That
could reduce the impact of this problem at least.

> Actually, "*cbranch_2insn" has the same logical as float "*cbranch" in ifcvt.
> Both of them get a final false return from "rs6000_emit_int_cmove" as rs6000
> doesn't have conditional move for FP mode.

I am about to commit patches for that.  But only for p10 and later.
It should eventually work for everything with isel (setbc can often be
optimised to isel after all), but the compiler has to work without isel
as well of course.

> So I think "cbranchcc4" should include "extra_insn_branch_comparison_operator"
> as "*cbranch_2insn" is a valid insn. Just let ifcvt decide a conditional
> move is valid or not.

It makes a bad decision though.  This is not okay.


Segher