Page MenuHomePhabricator

[SelectionDAGBuilder][CGP][X86] Move some of SDB's gather/scatter uniform base handling to CGP.

Authored by craig.topper on Mar 27 2020, 12:58 PM.



I've always found the "findValue" a little odd and
inconsistent with other things in SDB.

This simplies the code in SDB to just handle a splat constant
address or a 2 operand GEP in the same BB. This removes the
need for "findValue" since the operands to the GEP are
guaranteed to be available. The splat constant handling is
new, but was needed to avoid regressions due to constant
folding combining GEPs created in CGP.

CGP is now responsible for canonicalizing gather/scatters into
this form. The pattern I'm using for scalarizing, a scalar GEP
followed by a GEP with an all zeroes index, seems to be subject
to constant folding that the insertelement+shufflevector was not.

Diff Detail

Event Timeline

craig.topper created this revision.Mar 27 2020, 12:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2020, 12:58 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
spatel added inline comments.Apr 8 2020, 5:44 AM

I haven't looked at this before, so I'm not the best reviewer...
But it would be helpful to any first-time reader if this function had a documentation comment to describe the transform. An IR example in the comment and IR regression tests would also be educational.


Can this copied chunk be lifted to a helper function as a preliminary commit?

-Add IR tests
-Add comment to function.
-Simplify node deletion code. We don't have the iterator invalidation problem because we don't walk through phis.

The general approach here makes sense.


Does it matter whether the value is specifically zero, as opposed to an arbitrary splat value?


I think it would make sense to unify the handling where the last index is a scalar/splat, rather that splitting it based on whether GTI.isStruct() is true.

2886 ↗(On Diff #256134)

Not sure this is an improvement.


Orthogonal, but probably you could add a special-case here: vgather with an all-zero vector is equivalent to vbroadcast, I think?

craig.topper marked 3 inline comments as done.Apr 15 2020, 3:08 PM
craig.topper added inline comments.

It shouldn't matter, but I was just trying to avoid changing too much relative to the existing SelectionDAGBuilder code in this patch.


Good point. I added the scalar/splat case later after facing some regressions.

2886 ↗(On Diff #256134)

I think this was from debugging something and I didn't mean to leave it in here. I'll remove it.

Remove accidental Verifier change.
Unify struct and splat last index handling.

craig.topper marked an inline comment as done.Apr 15 2020, 5:09 PM
craig.topper added inline comments.

Yep it is. Need to look at whether we need to do that in DAG combine with a new X86ISD opcode to carry the mask or if we can just pattern match it in isel.

Maybe a weird question, but should we restrict this to masked gather/scatter operands? It seems like this should be profitable for any vector GEP to avoid vector operations where possible.


GTI is unused?

Maybe a weird question, but should we restrict this to masked gather/scatter operands? It seems like this should be profitable for any vector GEP to avoid vector operations where possible.

That's probably true. I've wondered if the GEP visitor in SelectionDAGBuilder should only splat when it finds the first vector index instead of for any scalar component of a vector GEP. Of course that means we need to scalarize splat indices earlier so that SelectionDAGBuilder sees them. Or we could split the GEPs in CGP or earlier like we're doing here. Splitting vector GEPs before LSR could be interesting to give LSR a chance to optimize induction variable uses in the scalar part.

This revision is now accepted and ready to land.Apr 16 2020, 4:06 PM
This revision was automatically updated to reflect the committed changes.