On Mon, Jul 1, 2019 at 4:05 PM Joern Wolfgang Rennecke
<[email protected]> wrote:
>
> [Apologies if this is a duplicate, I'm unsure if my previous mail was
> delivered]
> On 01/07/19 12:38, Richard Biener wrote:
> > On Mon, Jul 1, 2019 at 1:22 PM Joern Wolfgang Rennecke
> > <[email protected]> wrote:
> >> The heuristic introduced for PR71016 prevents recognizing a max / min
> >> combo like it is used for
> >> saturation when followed by a conversion.
> >> The attached patch refines the heuristic to allow this case. Regression
> >> tested on x86_64-pc-linux-gnu .
> > Few style nits:
> >
> > ...
> >
> > also please check that 'lhs' is equal to gimple_assing_rhs1 (arg0_def_stmt)
> > otherwise you'd also allow MIN/MAX unrelated to the conversion detected.
>
> Like the attached patch?
Yes - minor nit:
+ if (!operand_equal_p
+ (lhs, gimple_assign_rhs1 (arg0_def_stmt), 0))
+ return NULL;
you can use
if (lhs != gimple_assign_rhs1 (arg0_def_stmt))
return NULL;
since SSA names are shared tree nodes.
OK with that change.
> >
> > On x86 I see the MIN_EXPR is already detected by GENERIC folding,
> > I wonder if that is required or if we can handle the case without in one
> > phiopt pass invocation as well.
> >
> tree_ssa_phiopt_worker is supposed to handle this by handling nested
> COND_EXPRs
> from the inside out:
>
> /* Search every basic block for COND_EXPR we may be able to optimize.
>
> We walk the blocks in order that guarantees that a block with
> a single predecessor is processed before the predecessor.
> This ensures that we collapse inner ifs before visiting the
> outer ones, and also that we do not try to visit a removed
> block. */
> bb_order = single_pred_before_succ_order ();
>
> However, I have no idea how to construct a testcase for this with the
> gimple folding in place.
>
> #define SAT(x) (x < 0 ? 0 : (x > 255 ? 255 : x))
>
> void
> foo (unsigned char *p, int i, int m)
> {
> *p = (m > SAT (i) ? m : SAT (i));
> }
>
> or the equivalent for MIN_EXPR, *.c.004.original already has only one
> conditional left.
Hmm. The testcase in your patch should be equal to
void
foo (unsigned char *p, int i)
{
// tem1 = MAX ((unsinged char)MIN (i, 255), 0)
unsigned char tem1;
if (i >= 0)
{
// tem2 = MIN (i, 255)
int tem2;
if (i < 255)
tem2 = i;
else
tem2 = 255;
// tem1 = (unsigned char) tem2;
tem1 = tem2;
}
else
tem1 = 0;
*p = tem1;
}
but for this I don't see any MIN/MAX recognition :/ Anyhow - probably
a stupid mistake on my side ;)
Richard.
>
