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
