[Mesa-dev] [PATCH] i965: Consider SEL.{GE, L} to be commutative operations.

[Matt Turner]

On Sun, Dec 21, 2014 at 7:50 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On Sunday, December 21, 2014 03:37:25 PM Matt Turner wrote:
>> ---
>> No shader-db changes, unfortunately.
>
> Unsurprising :) This patch doesn't implement what you meant.
>
> 1. instructions_match() checks a->conditional_mod == b->conditional_mod.
>    So you won't ever see mixed conditional mods in operands_match() or
>    is_expression_commutative().  I suspect this is why you see no changes.

I'm not expecting to CSE SELs with different conditional mods. Maybe
I'm misunderstanding.

> 2. It looks like your patch allows CSE of (a >= b) with (b >= a),
>    which are clearly different.  So any changes would be wrong.

I don't think so? Those operations aren't implemented with SEL. I
think thinking about this as SEL+conditional mod is more confusing
than just MIN/MAX.

The intention of this patch is to allow MAX(a, b) and MAX(b, a) to be
recognized as identical expressions. Same for MIN.

> What you want is to use brw_swap_cmod.  It's then trivial to handle /all/
> comparitors, not just L/GE.  I think what you want is:
>
> static bool
> instructions match(fs_inst *a, fs_inst *b)
> {
>    bool match = a->opcode == b->opcode &&
>                 a->saturate == b->saturate &&
>                 a->predicate == b->predicate &&
>                 a->predicate_inverse == b->predicate_inverse &&
>                 a->dst.type == b->dst.type &&
>                 a->sources == b->sources;
>
>    if (a->is_tex()) {
>        match = match &&
>                a->offset == b->offset &&
>                a->mlen == b->mlen &&
>                a->regs_written == b->regs_written &&
>                a->base_mrf == b->base_mrf &&
>                a->eot == b->eot &&
>                a->header_present == b->header_present &&
>                a->shadow_compare == b->shadow_compare);
>    }
>
>    if (!match)
>       return false;
>
>    /* Comparisons match if both the comparitor and operands are flipped. */
>    if (a->opcode == BRW_OPCODE_SEL &&
>        a->conditional_mod == brw_swap_cmod(b->conditional_mod)
>        a->src[0].equals(b->src[1]) && a->src[1].equals(b->src[0])) {
>       return true;
>    }
>
>    return a->conditional_mod == b->conditional_mod && operands_match(a, b);
> }
>
> I'm also not sure why you special case SEL - it seems like CMP would work fine
> as well, and would probably help even more.  Maybe just drop the opcode check.

I think you're confused. :)

I'm special casing SEL because it's used to implement MIN/MAX and MIN
and MAX are commutative operations.

You're right that we could probably consider things like CMP with
different conditional mods as identical expressions (I'm not sure, the
comparison rules for CMP are non-obvious when you consider NaNs), but
that's not related to this patch.

The IVB PRM says about SEL with .l/.ge (specifically those, since they
are used to implement MIN/MAX):

For a sel instruction with a .l or .ge conditional modifier, if one
source is NaN and the other not NaN, the
non-NaN source is the result. If both sources are NaNs, the result is
NaN. For all other conditional
modifiers, if either source is NaN then src1 is selected.

That's the behavior I suggested GLSL should adopt in another thread.
That's the inspiration for this patch.

Does that clear it up?