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.
Jakub
