[PATCH] D21909: [DSE]Enhance shorthening MemIntrinsic based on OverlapIntervals

[Chad Rosier via llvm-commits]

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

LGTM, with a few minor comments.


================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:823
@@ +822,3 @@
+
+  if (!((llvm::isPowerOf2_64(LaterOffset) &&
+         EarlierWriteAlign <= LaterOffset) ||
----------------
Would this look any better if the negation were pushed through?  E.g.,

  if (!(llvm::isPowerOf2_64(LaterOffset) && EarlierWriteAlign <= LaterOffset) &&
      !((EarlierWriteAlign != 0) && LaterOffset % EarlierWriteAlign == 0))
    return false;

I think that's the same logic.. :)

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:834
@@ +833,3 @@
+  int64_t NewLength = IsOverwriteEnd
+                          ? LaterOffset - EarlierOffset
+                          : EarlierSize - (LaterOffset - EarlierOffset);
----------------
Is this clang-formatted?  Spacing looks odd.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:914
@@ +913,3 @@
+    Changed = tryToShortenEnd(EarlierWrite, IntervalMap, EarlierStart, EarlierSize);
+    if (!IntervalMap.empty())
+      Changed |=
----------------
  if (IntervalMap.empty())
    continue;

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:1052
@@ -937,9 +1051,3 @@
                      isShortenableAtTheBeginning(DepWrite)))) {
-          // TODO: base this on the target vector size so that if the earlier
-          // store was too small to get vector writes anyway then its likely
-          // a good idea to shorten it
-          // Power of 2 vector writes are probably always a bad idea to optimize
-          // as any store/memset/memcpy is likely using vector instructions so
-          // shortening it to not vector size is likely to be slower
-          MemIntrinsic *DepIntrinsic = cast<MemIntrinsic>(DepWrite);
-          unsigned DepWriteAlign = DepIntrinsic->getAlignment();
+          assert(!EnablePartialOverwriteTracking && "Do not expect to perform "
+                                                    "when partial-overwrite "
----------------
Is this how clang-format formatted the string?

================
Comment at: test/Transforms/DeadStoreElimination/OverwriteStoreEnd.ll:106
@@ +105,3 @@
+
+  %base64_2= getelementptr inbounds i64, i64* %P, i64 2
+  %base64_3= getelementptr inbounds i64, i64* %P, i64 3
----------------
Please add a space between %base64_2 and =.

================
Comment at: test/Transforms/DeadStoreElimination/OverwriteStoreEnd.ll:107
@@ +106,3 @@
+  %base64_2= getelementptr inbounds i64, i64* %P, i64 2
+  %base64_3= getelementptr inbounds i64, i64* %P, i64 3
+
----------------
Please add a space between %base64_3 and =.


https://reviews.llvm.org/D21909