Re: Canonicalization of compares performed as side-effect operations

[Uros Bizjak <ubizjak at gmail dot com>]

On Tue, Aug 6, 2019 at 11:03 PM Richard Earnshaw (lists)
<Richard.Earnshaw@arm.com> wrote:
>
> On 06/08/2019 18:39, Uros Bizjak wrote:
> > Hello!
> >
> >> On Tue, Aug 06, 2019 at 05:49:17PM +0100, Richard Earnshaw (lists) wrote:
> >>> On 06/08/2019 17:39, Segher Boessenkool wrote:
> >>>>>> What's wrong with describing the canonical form in your MD?  You'll need
> >>>>>> some reversed condition code thingy, but that's it?
> >>>>>
> >>>>> It doesn't describe what the instruction does.  The negation has a side
> >>>>> effect of setting the flags, but the flags are swapped because the
> >>>>> side-effect comparison is swapped from a normal compare.  As I
> >>>>> mentioned, SELECT_CC_MODE doesn't help because it can't see the context
> >>>>> and the comparison just looks 'normal'.
> >>>>
> >>>> Sure, and we can work on making combine do what you want, but your existing
> >>>> pattern is *incorrect*.  It needs fixing, and probably before we do other
> >>>> things.
> >>>
> >>> Why is it incorrect?  It's not canonical, sure.  But the cannonical form
> >>> does NOT describe what the instruction does.
> >>
> >> More precisely: not having the canonical form of this in your MD is what
> >> is incorrect.
> >
> > There is TARGET_CANONICALIZE_COMPARISON target hook that can solve the
> > issue here. Please see x86, where we canonicalize
> > *cmp<X87MODEF:mode>_<SWI24:mode>_i387 via
> > ix86_canonicalize_comparison. As said in the comment:
> >
> >   /* The order of operands in x87 ficom compare is forced by combine in
> >      simplify_comparison () function. Float operator is treated as RTX_OBJ
> >      with a precedence over other operators and is always put in the first
> >      place. Swap condition and operands to match ficom instruction.  */
> >
> > The issue looks similar to me.
> >
>
> That hook can't help as it can't see anything other than the operands of
> the compare, and from the operands it looks as though the operands
> should be swapped.  To correctly canonicalize this you need the whole
> parallel, or some other hint that the operation is a side-effect of
> another operation.

I see. FYI, there is the same problem in i386.md with "*neg<mode>2_cmpz"

--cut here--
;; The problem with neg is that it does not perform (compare x 0),
;; it really performs (compare 0 x), which leaves us with the zero
;; flag being the only useful item.

(define_insn "*neg<mode>2_cmpz"
  [(set (reg:CCZ FLAGS_REG)
    (compare:CCZ
      (neg:SWI (match_operand:SWI 1 "nonimmediate_operand" "0"))
           (const_int 0)))
   (set (match_operand:SWI 0 "nonimmediate_operand" "=<r>m")
    (neg:SWI (match_dup 1)))]
--cut here--

However, when trying to generate the pattern using following testcase:

--cut here--
void foo (void);
void bar (void);

int
test (int a, int b)
{
  int r;

  if (r = -b)
    foo ();
  else
    bar ();

  return r;
}
--cut here--

combine doesn't even consider the combination, since the compare looks
at the original value:

(insn 7 4 8 2 (parallel [
            (set (reg/v:SI 82 [ <retval> ])
                (neg:SI (reg/v:SI 84 [ b ])))
            (clobber (reg:CC 17 flags))
        ]) "neg.c":9:9 461 {*negsi2_1}
     (expr_list:REG_UNUSED (reg:CC 17 flags)
        (nil)))
(insn 8 7 9 2 (set (reg:CCZ 17 flags)
        (compare:CCZ (reg/v:SI 84 [ b ])
            (const_int 0 [0]))) "neg.c":9:6 7 {*cmpsi_ccno_1}
     (expr_list:REG_DEAD (reg/v:SI 84 [ b ])
        (nil)))

probably due to tree optimizers that give us:

  r_3 = -b_2(D);
  if (b_2(D) != 0)
    goto <bb 3>; [50.00%]
  else
    goto <bb 4>; [50.00%]

Uros.