On Wed, 4 Jan 2017, Jeff Law wrote:
> On 01/04/2017 11:01 AM, Jakub Jelinek wrote:
> > On Wed, Jan 04, 2017 at 11:19:46AM +0100, Richard Biener wrote:
> > > For the SSA_NAME + INTEGER_CST case restrict it to the case
> > >
> > > if (x_1 > 5)
> > > tem_2 = (char) x_1;
> > > # tem_3 = PHI <tem_2, 5>
> > >
> > > that is, (char) x_1 uses x_1 and that also appears in the controlling
> > > GIMPLE_COND. That's what enables followup minmax replacement
> > > (gcc.dg/tree-ssa/pr66726.c). Another case where it might be
> > > profitable is if the BB is empty after the conversion is sunk
> > > (may enable other value-replacement cases).
> >
> > Do we actually have any testcases where we need the SSA_NAME + INTEGER_CST
> > case?
> > The only testcase that has been added that actually relied on the
> > factor_out_* is since r242750 handled during gimple folding (already in
> > *.gimple dump).
> >
> > If I modify the testcase so that it isn't recognized by the minmax folding,
> > extern unsigned short y[];
> >
> > int
> > foo (int x)
> > {
> > return 64 < y[x] ? 68 : y[x];
> > }
> >
> > then vanilla gcc vs. gcc with factor_out* INTEGER_CST case removed gives:
> > - cmpw $65, %ax
> > - cmovnb %edx, %eax
> > + cmpw $64, %ax
> > + cmova %edx, %eax
> > so not worse or better. And, even when the conversion is the only stmt
> > in the basic block, that still doesn't allow the bb to be removed, we need
> > the empty bb for the PHIs.
> My recollection is this was a piece of the puzzle towards solving 45397.
> Given that we haven't solved 45397 yet, I won't object if we want to reject
> SSA_NAME + INTEGER_CST at this time and come back to it when we dig into 45397
> again.
I think that would be premature. Consider a modified
gcc.dg/tree-ssa/pr66726.c:
extern unsigned short mode_size[];
int
oof (int mode)
{
int tem;
if (64 < mode_size[mode])
tem = 64;
else
tem = mode_size[mode];
return tem;
}
here phiopt1 manages to produce
oof (int mode)
{
int tem;
short unsigned int _1;
short unsigned int _7;
<bb 2> [100.00%]:
_1 = mode_size[mode_4(D)];
_7 = MIN_EXPR <_1, 64>;
tem_2 = (int) _7;
return tem_2;
which is an important simplification for example for vectorization or
other loop opts requiring a simple loop structure.
That we now fold the GENERIC cond-expr during gimplification
is of course good but doesn't make the phiopt code useless.
As far as I remember I objected adding handling of conversions
in the minmax replacement code and instead suggested to add this
"enablement" phiopt instead. So what I suggest is to tame that down
to the cases where it actually enables something (empty BB afterwards,
a PHI with a use that's also used on the controlling conditional).
Richard.
