[PATCH] Masked Vector Load/Store Intrinsics

[hfinkel at anl.gov]

Thanks for working on this. A bunch of these things can be split up into different patches (even if we don't do that for the review, please make sure you split them when committing).

You're missing separate cost-model tests for the cost-model changes. These can (and should) be tested independently from the vectorizer tests.

================
Comment at: include/llvm/CodeGen/SelectionDAGNodes.h:1933
@@ +1932,3 @@
+/// MaskedLoadSDNode - This class is used to represent ISD::MLOAD nodes.
+///
+class MaskedLoadStoreSDNode : public MemSDNode {
----------------
Remove empty comment line (same for classes below).

================
Comment at: include/llvm/CodeGen/SelectionDAGNodes.h:1935
@@ +1934,3 @@
+class MaskedLoadStoreSDNode : public MemSDNode {
+
+  // Operands
----------------
Remove line. (same for classes below)

================
Comment at: include/llvm/CodeGen/SelectionDAGNodes.h:1946
@@ +1945,3 @@
+  }
+  //const SDValue &getOffset() const {
+  //  return getOperand(getOpcode() == ISD::MLOAD ? 2 : 3);
----------------
Remove commented-out code.

================
Comment at: include/llvm/CodeGen/SelectionDAGNodes.h:1973
@@ +1972,3 @@
+
+  const SDValue &getBasePtr() const { return getOperand(1); }
+  const SDValue &getMask() const { return getOperand(2); }
----------------
Don't repeat this here -- it is in the base class.

================
Comment at: include/llvm/CodeGen/SelectionDAGNodes.h:1974
@@ +1973,3 @@
+  const SDValue &getBasePtr() const { return getOperand(1); }
+  const SDValue &getMask() const { return getOperand(2); }
+  const SDValue &getSrc0() const { return getOperand(3); }
----------------
All subclasses of MaskedLoadStoreSDNode will have a getMask -- move that into the base class too. Add a comment about the data type of the mask.

================
Comment at: include/llvm/IR/IRBuilder.h:434
@@ +433,3 @@
+  /// Masked intrinsic has only one overloaded type - data type.
+  CallInst *CreateMaskedIntrinsic(unsigned Id, ArrayRef<Value *> Ops,
+                                  Type *DataTy);
----------------
The pattern of this class interface is to have a function per intrinsic, so that the intrinsic id and operand structure can be hidden within the implementation. Please split this into CreateMaskedLoad and CreateMaskedStore, each with appropriate parameters.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4787
@@ +4786,3 @@
+    // Check if any splitting is required.
+    if (TLI.getTypeAction(*DAG.getContext(), Data.getValueType()) !=
+        TargetLowering::TypeSplitVector)
----------------
Vector splitting is a legalization action, why is this being handled here in DAGCombine (as opposed to in LegalizeVectorTypes) -- shouldn't this be a part of DAGTypeLegalizer::SplitVecRes_MSTORE?

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4848
@@ +4847,3 @@
+    // Check if any splitting is required.
+    if (TLI.getTypeAction(*DAG.getContext(), VT) !=
+        TargetLowering::TypeSplitVector)
----------------
Same question as above.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:1021
@@ +1020,3 @@
+                         MachineMemOperand::MOLoad,  HiMemVT.getStoreSize(),
+                         Alignment, MLD->getAAInfo(), MLD->getRanges());
+
----------------
This alignment is not right -- if the full load as alignment Alignment, then the upper half can have only half that.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:1489
@@ +1488,3 @@
+                         MachineMemOperand::MOStore,  HiMemVT.getStoreSize(),
+                         Alignment, N->getAAInfo(), N->getRanges());
+
----------------
Same comment re: the alignment.

================
Comment at: lib/IR/IRBuilder.cpp:176
@@ -175,2 +175,3 @@
 
+
 CallInst *IRBuilderBase::CreateAssumption(Value *Cond) {
----------------
Don't add blank line.

================
Comment at: lib/Target/X86/X86TargetTransformInfo.cpp:623
@@ -620,1 +622,3 @@
 
+    { ISD::SINT_TO_FP,  MVT::v16f32, MVT::v16i1,  2 },
+    { ISD::SINT_TO_FP,  MVT::v16f32, MVT::v16i8,  2 },
----------------
This is a separate change, than can be committed and tested separately.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5341
@@ -5340,3 +5406,2 @@
   }
-
   return true;
----------------
Don't remove this line.

================
Comment at: test/CodeGen/X86/masked_memop.ll:66
@@ +65,3 @@
+}
+
+
----------------
Are there tests in this file that demonstrate complete scalarization? If not, please add some. If so, please note them.

http://reviews.llvm.org/D6191