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 <guih...@linux.ibm.com> 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