Page MenuHomePhabricator

[SDAG] When performing post-legalize DAG combining, run the legalizer over each node in the worklist prior to combining.

Authored by chandlerc on Jul 17 2014, 9:54 AM.



This allows the combiner to produce new nodes which need to go back
through legalization. This is particularly useful when generating
operands to target specific nodes in a post-legalize DAG combine where
the operands are significantly easier to express as pre-legalized
operations. My immediate use case will be PSHUFB formation where we need
to build a constant shuffle mask with a build_vector node.

This also refactors the relevant functionality in the legalizer to
support this, and updates relevant tests. I've spoken to the R600 folks
and these changes look like improvements. The avx512 change needs to be
investigated, I suspect there is a disagreement between the legalizer
and the DAG combiner there, but it seems a minor issue so leaving it to
be re-evaluated after this patch.

Diff Detail


Event Timeline

chandlerc updated this revision to Diff 11583.Jul 17 2014, 9:54 AM
chandlerc retitled this revision from to [SDAG] When performing post-legalize DAG combining, run the legalizer over each node in the worklist prior to combining..
chandlerc updated this object.
chandlerc edited the test plan for this revision. (Show Details)
chandlerc added reviewers: hfinkel, grosbach.
chandlerc added a subscriber: Unknown Object (MLST).
filcab added a subscriber: filcab.Jul 17 2014, 5:48 PM

It's “nice” to see we weren't copying debug info (although many combines and lowerings try to keep the original info).

chandlerc updated this revision to Diff 11862.Jul 24 2014, 5:36 PM

Reflect the changes to the combiner that allowed this to handle more edge
cases. Also simplify some of the now unnecessary book keeping that was papering
over the flaws in the combiner itself.

FYI, I think this is essentially ready-to-go. Any final comments?

hfinkel accepted this revision.Jul 25 2014, 5:12 PM
hfinkel edited edge metadata.


185 ↗(On Diff #11862)

Is this a fly-by fix, or something that was not needed previously?

4313 ↗(On Diff #11862)

I actually dislike this comment because it calls something hacky without explaining why or what should be done instead.

4320 ↗(On Diff #11862)

If we could get a vector scalarization here, I'd make this 64 instead of 16.

This revision is now accepted and ready to land.Jul 25 2014, 5:12 PM

Thanks Hal, submitting with the fixes described below.

185 ↗(On Diff #11862)

Fly-by fix, and sadly neither Dave nor I had any good ideas about how to test. =/

4313 ↗(On Diff #11862)

Yea, sorry, this comment was supposed to get deleted. It isn't hacky at all at this point I think, it dates from when I didn't even think this approach would work.

There is a proper doxygen comment in the header already. However, that comment was pretty dated as well. I've beefed it up to clarify a lot of the subtle semantic contracts here.

4320 ↗(On Diff #11862)

I don't disagree completely, but the one that legalizes the *entire DAG* has only 16, so it seems weird to make this one larger. I'm going to keep this as-is for now, and maybe one of us can get measurements to drive a change here and/or above...

I'm also really frustrated by having both UpdatedNodes and LegalizedNodes. I'd like to eventually remove one of them, but haven't yet found a way.

chandlerc closed this revision.Jul 25 2014, 10:58 PM
chandlerc updated this revision to Diff 11911.

Closed by commit rL214020 (authored by @chandlerc).