[PATCH] D20262: [DSE]Split memset when the memset is small enough to be lowered to stores

[Quentin Colombet via llvm-commits]

qcolombet added inline comments.

================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:465
@@ +464,3 @@
+  /// that have OptSize attribute.
+  unsigned getMaxStoresPerMemset(bool OptSize) const;
+
----------------
What about the MinSize attribute? (Oz)
Shouldn’t we just pass the Function (or its directly its list of attribute) and let the target checks whatever attribute it feels appropriate?

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:238
@@ +237,3 @@
+
+  if (MaxIntSize == 0)
+    MaxIntSize = 1;
----------------
junbuml wrote:
> dberlin wrote:
> > How and when can this happen?
> If there is no information about LegalIntWidth extracted from datalayout, this could be 0.  If we remove the target datalayout in input IR, this will be 0.
Like Daniel said, DL are mandatory now. If that may still happen, (int would be an illegal type??) add a comment on when this is the case and add a test case for that!

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:241
@@ +240,3 @@
+
+  if (OrigLength <= MaxIntSize)
+    return false;
----------------
Add a comment why this is not profitable. Something along the line of “this is going to be only one store”.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:439
@@ +438,3 @@
+
+  if (LaterOff > EarlierOff &&
+      int64_t(LaterOff + Later.Size) < int64_t(EarlierOff + Earlier.Size)) {
----------------
Add a comment with ASCII art like the other cases to explain what we handle here.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:1009
@@ +1008,3 @@
+              MadeChange = true;
+            }
+          }
----------------
Make a method out of this to break the nested indentation.

================
Comment at: test/Transforms/DeadStoreElimination/SplitMemintrinsic.ll:1
@@ +1,2 @@
+; RUN: opt < %s -basicaa -dse -mtriple=arm64-linux-gnu -S | llc -mtriple=arm64-linux-gnu  | FileCheck %s
+
----------------
I would prefer two different tests:
1. One that checks that dse does what is expected.
2. One that checks that given such input (statically available, not produced by opt), the backend does what is expected.

================
Comment at: test/Transforms/DeadStoreElimination/SplitMemintrinsic.ll:78
@@ +77,3 @@
+entry:
+  %0 = bitcast i64* %P to i8*
+  call void @llvm.memset.p0i8.i64(i8* %0, i8 0, i64 34, i32 8, i1 false)
----------------
Use opt -instnamer


http://reviews.llvm.org/D20262