[PATCH] D60318: [ExpandMemCmp][MergeICmps] Move passes out of CodeGen into opt pipeline.

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

LGTM - see inline for a few more nits.

I encourage adding small memcmp tests to test-suite as a follow-up, so we know that things won't break going forward.

I haven't seen the DomTreeUpdater API before now, so I'm assuming the tests are verifying that we made the correct updates (and the earlier rL361239 <https://reviews.llvm.org/rL361239> has survived in trunk).



================
Comment at: llvm/lib/Transforms/Scalar/ExpandMemCmp.cpp:309-312
     BranchInst *CmpBr =
-        BranchInst::Create(EndBlock, LoadCmpBlocks[BlockIndex + 1], Cmp);
+        BranchInst::Create(EndBlock, NextBB, Cmp);
     Builder.Insert(CmpBr);
+    DTU.applyUpdates({{DominatorTree::Insert, BB, EndBlock}, {DominatorTree::Insert, BB, NextBB}});
----------------
Formatting is off here - line that fits 80-col is split, and line that doesn't fit is not split.


================
Comment at: llvm/lib/Transforms/Scalar/ExpandMemCmp.cpp:434
   }
+  DTU.applyUpdates({{DominatorTree::Insert, BB, ResBlock.BB}, {DominatorTree::Insert, BB, NextBB}});
 }
----------------
Formatting - split line.


================
Comment at: llvm/lib/Transforms/Scalar/ExpandMemCmp.cpp:507
   }
+  DTU.applyUpdates({{DominatorTree::Insert, BB, ResBlock.BB}, {DominatorTree::Insert, BB, NextBB}});
 }
----------------
Formatting - split line.


================
Comment at: llvm/lib/Transforms/Scalar/ExpandMemCmp.cpp:649
+    StartBlock->getTerminator()->setSuccessor(0, FirstLoadBB);
+    DTU.applyUpdates({{DominatorTree::Delete, StartBlock, EndBlock}, {DominatorTree::Insert, StartBlock, FirstLoadBB}});
   }
----------------
Formatting - split line.


================
Comment at: llvm/test/Transforms/PhaseOrdering/X86/memcmp.ll:5
+
+; This tests codegen time inlining/optimization of memcmp
+; rdar://6480398
----------------
Update stale comment:
'This tests interaction between the MergeICmp and ExpandMemCmp IR transform passes.'


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60318/new/

https://reviews.llvm.org/D60318