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

Chandler Carruth chandlerc at gmail.com
Fri Jul 25 22:53:32 PDT 2014


Thanks Hal, submitting with the fixes described below.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:185
@@ -171,1 +184,3 @@
+    for (unsigned i = 0, e = Old->getNumValues(); i != e; ++i)
+      DAG.TransferDbgValues(SDValue(Old, i), SDValue(New, i));
     ReplacedNode(Old);
----------------
hfinkel at anl.gov wrote:
> Is this a fly-by fix, or something that was not needed previously?
Fly-by fix, and sadly neither Dave nor I had any good ideas about how to test. =/

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:4313
@@ +4312,3 @@
+
+// This is a somewhat hacky entry point to legalize a single sub-graph rooted
+// at a node. This is used by the post-legalize DAG combiner to allow its DAG
----------------
hfinkel at anl.gov wrote:
> I actually dislike this comment because it calls something hacky without explaining why or what should be done instead.
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.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:4320
@@ +4319,3 @@
+  allnodes_iterator LegalizePosition(N);
+  SmallPtrSet<SDNode *, 16> LegalizedNodes;
+  SelectionDAGLegalize Legalizer(*this, LegalizePosition, LegalizedNodes,
----------------
hfinkel at anl.gov wrote:
> If we could get a vector scalarization here, I'd make this 64 instead of 16.
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.

http://reviews.llvm.org/D4564






More information about the llvm-commits mailing list