[PATCH] D13855: Masked load/store optimization for scalar code

[Michael Kuperstein via llvm-commits]

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

LGTM with a few nits.


================
Comment at: ../lib/CodeGen/CodeGenPrepare.cpp:1142
@@ -1138,1 +1141,3 @@
 
+  // Shorten the way if the mask is all-true.
+  bool IsAllOnesMask = isa<Constant>(Mask) &&
----------------
"Shorten the way" -> "Short-cut"

================
Comment at: ../lib/CodeGen/CodeGenPrepare.cpp:1154
@@ -1139,1 +1153,3 @@
+  // Adjust alignment for the scalar instruction.
+  AlignVal = std::min(AlignVal, VecType->getScalarSizeInBits()/8);
   // Bitcast %addr fron i8* to EltTy*
----------------
You can load the first scalar with the original alignment, right? (Not that I'm sure it matters any)

================
Comment at: ../lib/CodeGen/CodeGenPrepare.cpp:1166
@@ -1147,1 +1165,3 @@
 
+  bool IsConstMask = isa<ConstantVector>(Mask);
+  if (IsConstMask) {
----------------
Why do you need the temp variable? I think it's just as clear with "if (isa<ConstantVector>Mask)) { ..."
As a different option, you could have
ConstantVector *ConstMask = dyn_cast<ConstantVector>(Mask)

And then use ConstMask in both lines 1167 and 1169.

================
Comment at: ../lib/CodeGen/CodeGenPrepare.cpp:1286
@@ -1249,1 +1285,3 @@
 
+  // Shorten the way if the mask is all-true.
+  bool IsAllOnesMask = isa<Constant>(Mask) &&
----------------
Same as above.

================
Comment at: ../lib/CodeGen/CodeGenPrepare.cpp:1304
@@ +1303,3 @@
+
+  bool IsConstMask = isa<ConstantVector>(Mask);
+  if (IsConstMask) {
----------------
Same as above.


Repository:
  rL LLVM

http://reviews.llvm.org/D13855