Re: ifcvt vs. expand_binop

[Oleg Endo <oleg dot endo at t-online dot de>]

Hi,

On Tue, 2015-09-22 at 15:21 +0100, Kyrill Tkachov wrote:

> where does noce_emit_store_flag call expand_simple_binop?
> Do you mean the code following the call to noce_emit_store_flag
> in noce_try_store_flag_constants? (I suspect that's the code that
> will get triggered for your testcase)

Sorry, forgot to mention that.  Yes, it's in
noce_try_store_flag_constants:

  /* Always use ifalse here.  It should have been swapped with itrue
     when appropriate when reversep is true.  */
  target = expand_simple_binop (mode, subtract_flag_p ? MINUS : PLUS,


> > The problem is that expand_binop gets confused, seemingly because
> > op1 and target are the same.  It tries to emit a 64 bit addition:
> >
> > (insn 50 49 51 (clobber (reg:DI 173)) -1
> >       (nil))
> >
> > (insn 51 50 52 (set (subreg:SI (reg:DI 173) 0)
> >          (reg:SI 162)) -1
> >       (nil))
> >
> > (insn 52 51 53 (set (reg:DI 175)
> >          (const_int 2147483647 [0x7fffffff])) -1
> >       (nil))
> >
> > (insn 53 52 0 (parallel [
> >              (set (reg:DI 174)
> >                  (plus:DI (reg:DI 173)
> >                      (reg:DI 175)))
> >              (clobber (reg:SI 147 t))
> >          ]) -1
> >       (nil))
> 
> huh, why does it try to do that? I'll admit I don't know the details
> of how that expansion works. Presumably it's very target-specific

I haven't checked the details.  But I guess because expand_binop wants
to somehow reuse the input and output it creates a DImode pseudo, puts
the input there, does the DImode plus and the returned "target" is a
SImode subreg of the DImode result.  The strange thing is the first
clobber (insn 50)... Maybe there's some other hidden problem in
expand_binop which is triggered by ifcvt in this case.

Even if the DImode plus worked OK, I guess the result would be counter
productive.

> Looks reasonable to me with one caveat.
> The comment text for noce_emit_store_flag is not very helpful

Which comment do you mean?  I don't see any ...

> and my first expectation of the X parameter to it is that it is the
> target where the flag is stored.

If the "return emit_store_flag" path in noce_emit_store_flag is taken,
it already returns a new reg rtx.  At least this is happening on SH
right now, because the "short cut path" is not taken.

>  With your case this will no longer
> be the case (emit_store_flag seems to attempt to return the result in
> x, judging by its comment in expmed.c). 

Hm yeah, but I guess there's no guarantee to that.  In emit_store_flag_1
(used by emit_store_flag), there's this:

  rtx tem = emit_cstore (target, icode, code, mode, compare_mode,
			 unsignedp, op0, op1, normalizep, target_mode);

And this might return a new reg with the output.

> I think it would be a good idea
> to expand the comment of noce_emit_store_flag to describe its arguments
> and to mention that the result is returned but is not necessarily placed in X.

Sure, I could add that.

> I think all the callers in ifcvt.c already guard against that possibility and
> do something along the lines of:
>   target = noce_emit_store_flag (..., x, ..., ...);
>   if (target != if_info->x)
>     noce_emit_move_insn (if_info->x, target);

Not in all the places.  It seems "target" is copied into "if_info->x" at
the very end of the sequence, which makes sense.  In between there are
several places where "if_info->x" is passed as the proposed target to
noce_emit_store_flag, but the following code always uses the actual
result reg.  At least as far as I can see. 

> but it would be nice to go through the callers and make sure that they don't assume
> that the result of noce_emit_store_flag is in always x.

It looks OK.  At least I don't see anything suspicious.  But it's better
if somebody else also takes a peek.

Cheers,
Oleg