Hi guys,
On Tue, Nov 15, 2022 at 10:04:04PM -0500, David Edelsohn wrote:
> On Tue, Nov 15, 2022 at 9:32 PM HAO CHEN GUI <[email protected]> wrote:
> > The patch enables have_cbrnachcc4 which is a flag in ifcvt.cc to
> > indicate if branch by CC bits is invalid or not. As rs6000 already has
> > "*cbranch" insn which does branching according to CC bits, the flag
> > should be enabled and relevant branches can be optimized out. The test
> > case illustrates the optimization.
cbranch<mode>4 normally compares the <mode>mode operands 2 and 3, and
jumps to operands[0] if operands[1] (a relation of operands 2 and 3) is
true. See emit_cmp_and_jump_insns for example. But cbranchcc4 does
not do any comparison; instead, it branches based on the result of a
previous comparison, stored in operands[2], and operands[3] has to be
const_int 0.
This really should never have been done this way, because now almost
everywhere we need exception code for MODE_CC things here (and always
do only the exceptional case on targets that have no use for actual
compare-and-branch patterns).
Another appearance of this (last stage 4!) was PR104335. We never
followed up on that, probably should have, sigh.
So, cbranch<mode>4 now means "compare and branch", except when <mode> is
cc, in which case it means "conditional branch", and operands[3] is
required to be const_int 0 in that case, and if not we ICE, or worse, do
the wrong thing. Woohoo.
> > "*cbranch" is an anonymous insn which can't be generated directly.
It can be, rs6000_emit_cbranch (which does mean "conditional branch"
btw). All RTL can be created out of thin air.
But you mean "can't be generated by name"? Yup.
> > So changing "const_int 0" to the third operand predicated by
> > "zero_constant" won't cause ICEs as orginal patterns still can be matched.
It never is valid to have an actual comparison of anything MODE_CC with
anything else, only MODE_INT, MODE_FLOAT, MODE_DECIMAL_FLOAT have a
(total) order. The (const_int 0) in such MODE_CC RTL is only notational
convenience, it does not mean integer 0 or anything else, it is a
placeholder so things can look like a relation.
> > -(define_insn "*cbranch"
> > +(define_insn "cbranchcc4"
> > [(set (pc)
> > (if_then_else (match_operator 1 "branch_comparison_operator"
> > [(match_operand 2 "cc_reg_operand"
> > "y")
> > - (const_int 0)])
> > + (match_operand 3 "zero_constant")])
> > (label_ref (match_operand 0))
> > (pc)))]
> > ""
> Shouldn't cbranchcc4 be a separate pattern? This pattern has an existing
> purpose and an expected ordering of operands.
It still has the same operands, only operands[3] is added. Nothing can
create the old pattern by name (because its name is starred), so the
expander won't ever access operands[3] while there are only 3 operands
passed -- simply because the expander cannot be called in any way :-)
> cbranchcc4 is passed the comparison operator as operand 0. Other ports
> ignore the second comparison operator and use (const_int 0). Operand 3 is
> the label, which seems to be the reason that you needed to change it to
> match_operand 3.
operands[0] is the dest, operands[1] is the relation, operands[2] is the
cc reg, operands[3] is const_int 0. The dest is last in the pattern, to
make it more confusing to read :-)
> It's great to add cbranchcc4 to the Power port where it definitely was an
> omission, but adapting *cbranch for that purpose is the wrong approach.
> The changes to the pattern are incorrect because they are covering up a
> difference in ordering of the operands. One can argue that the named
> pattern only enables the functionality in ifcvt and the pattern otherwise
> is used in its previous role. But this is a Frankenstein monster
> approach.
I agree with that! But the generic code adopted that, and other ports
followed it as well, so who are we to disdain this nice performance
improvement?
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/powerpc/cbranchcc4.c
> > @@ -0,0 +1,8 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2 -fdump-rtl-ce1" } */
> > +/* { dg-final {scan-rtl-dump "noce_try_store_flag_constants" "ce1" } } */
(space after { please? For consistency.)
> > +int test (unsigned int a, unsigned int b)
> > +{
> > + return (a < b ? 0 : (a > b ? 2 : 1));
> > +}
Please add a comment what this test wants to test for? Something like
/* If-conversion should <something here>. */
maybe?
I would approve this patch, given that the compiler will immediately ICE
if something not a zero is passed as operands[3] (although it would be
nice to have a zero_int_operand that would aonly allow const_int).
David, do you agree given all of the above?
Thanks,
Segher
