[PATCH] D19821: [EarlyCSE] Port to use MemorySSA (disabled by default). NFC.

Philip Reames via llvm-commits llvm-commits at lists.llvm.org
Thu May 5 18:31:24 PDT 2016


reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

At a meta level, I'm not convinced that updating EarlyCSE to work with MemorySSA is the right approach.  EarlyCSE is focused on being really really fast at cleaning up stupidly redundant IR so that the rest of the pass pipeline doesn't need to worry about it.  MemorySSA is relatively expensive to construct.  Given that, I'm really not sure putting it at the very beginning of the pipeline is a good design choice.

Now, having said that, it might make sense to have a dominance order CSE pass based on MemorySSA for use later in the pass pipeline.  Currently we use EarlyCSE for two distinct purposes, its possible that it might be time to split them.

Can you justify why this is the right approach?


================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:511
@@ +510,3 @@
+                                       MemorySSAWalker *MSSAWalker) {
+  if (!CanSkipReleaseFence)
+    return MA;
----------------
Huh?  This should be handled entirely inside MemorySSA?

================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:562
@@ +561,3 @@
+
+  MemoryAccess *LaterHeapGen =
+      getHeapGeneration(LaterInst, CanSkipReleaseFence, MSSA, MSSAWalker);
----------------
Having two sets of variables, one integers, one pointers with similar names is *highly* confusing.  I'd suggest pulling out a MemorySSA specific impl function and calling it from here to wrap the desired asserts.

================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:570
@@ +569,3 @@
+
+  // handle case of e.g. load atomic -> store unordered
+  if (LaterHeapGen == EarlierMA)
----------------
mcrosier wrote:
> Please capitalize handle and add a period.
This comment doesn't make sense where placed?

================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:682
@@ -578,1 +681,3 @@
+      // get removed in the code above and MemorySSA updated correctly.
+      removeMSSA(Inst);
       Inst->eraseFromParent();
----------------
Code like this strongly hints that MemorySSA should be using ValueHandles.

================
Comment at: test/Transforms/EarlyCSE/memoryssa.ll:7
@@ +6,3 @@
+;; Simple load value numbering across non-clobbering store.
+; CHECK-LABEL: @test1(
+define i32 @test1() {
----------------
If we do go this way, you'll need far far more tests.


http://reviews.llvm.org/D19821





More information about the llvm-commits mailing list