[PATCH] D13722: Scalarization for masked gather/scatter intrinsics

[Michael Kuperstein via llvm-commits]

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

LGTM with some nits.


================
Comment at: ../lib/CodeGen/CodeGenPrepare.cpp:1129
@@ -1128,3 +1128,3 @@
   assert(VecType && "Unexpected return type of masked load intrinsic");
 
   IRBuilder<> Builder(CI->getContext());
----------------
That's ok, I'm just saying I would prefer not to have an unchecked dyn_cast<>.

================
Comment at: ../lib/CodeGen/CodeGenPrepare.cpp:1361
@@ +1360,3 @@
+                                                "Ptr" + Twine(Idx));
+      LoadInst* Load = Builder.CreateAlignedLoad(Ptr, AlignVal,
+                                                 "Load" + Twine(Idx));
----------------
Don't you need AlignVal = std::min(AlignVal, VecType->getScalarSizeInBits()), like for the load/store case?

================
Comment at: ../lib/CodeGen/CodeGenPrepare.cpp:1367
@@ +1366,3 @@
+    }
+    Value *NewI = Builder.CreateSelect(Mask, VResult, Src0);
+    CI->replaceAllUsesWith(NewI);
----------------
This gets eventually cleaned up is Src0 is undef, right?

================
Comment at: ../lib/CodeGen/CodeGenPrepare.cpp:1410
@@ +1409,3 @@
+                                              "Ptr" + Twine(Idx));
+    LoadInst* Load = Builder.CreateAlignedLoad(Ptr, AlignVal,
+                                               "Load" + Twine(Idx));
----------------
LoadInst* Load -> LoadInst *Load

================
Comment at: ../lib/CodeGen/CodeGenPrepare.cpp:1493
@@ +1492,3 @@
+                                                "Ptr" + Twine(Idx));
+      Builder.CreateAlignedStore(OneElt, Ptr, AlignVal);
+    }
----------------
Same as above re AlignVal

================
Comment at: ../lib/Target/X86/X86TargetTransformInfo.cpp:1212
@@ +1211,3 @@
+}
+
+bool X86TTIImpl::isLegalMaskedScatter(Type *DataType) {
----------------
So, say, a <32 x i32> gather will get split into two <16 x i32> gathers by the legalizer?
In any case:
a) We should probably reject the not-power-of-2 case, unless we know the legalizer can handle it. If we don't reject it, then I think there should be a test for it.
b) I guess the name is a bit confusing - it's not really "isLegal" (because some things it accepts may not actually be legal on the target), it's more like "isLegalizeable". I don't think we should change the name, but it may be worth to note this in the declaration in TargetTransformInfo.h.


Repository:
  rL LLVM

http://reviews.llvm.org/D13722