On Fri, 17 Apr 2020, Jakub Jelinek wrote: > On Fri, Apr 17, 2020 at 09:39:20AM +0200, Richard Biener wrote: > > On Fri, 17 Apr 2020, Jakub Jelinek wrote: > > > > > On Fri, Apr 17, 2020 at 09:24:34AM +0200, Richard Biener wrote: > > > > --- a/gcc/optabs.c > > > > +++ b/gcc/optabs.c > > > > @@ -1050,7 +1050,7 @@ expand_binop_directly (enum insn_code icode, > > > > machine_mode mode, optab binoptab, > > > > commutative_p = commutative_optab_p (binoptab); > > > > if (commutative_p > > > > && GET_MODE (xop0) != xmode0 && GET_MODE (xop1) != xmode1 > > > > - && GET_MODE (xop0) == xmode1 && GET_MODE (xop1) == xmode1) > > > > + && GET_MODE (xop0) == xmode1 && GET_MODE (xop1) == xmode0) > > > > > > Do we need 4 comparisons for that? Wouldn't > > > if (commutative_p > > > && xmode0 != xmode1 > > > && GET_MODE (xop0) == xmode1 > > > && GET_MODE (xop1) == xmode0) > > > std::swap (xop0, xop1); > > > be more readable and shorter? > > > > Not sure - xmode[01] are derived from insn_data while xop[01] are > > passed in as arguments so it seems the code wants to check the > > modes are not fine already, not whether swapping is a no-op > > because both are equal. > > I meant that the condition after your changes is actually functionally > equivalent to the one I've posted. > Because, if xmode0 == xmode1, then the condition will never be true, > as either GET_MODE (xop0) == xmode1 or GET_MODE (xop0) != xmode0 will > be false. And if xmode0 != xmode1, we have the two == comparisons, > so no need to check further that it is unequal to something else.
OK, that's true. Not sure about being more readable though. Richard.
