[PATCH] D17270: Support arbitrary addrspace pointers in masked load/store intrinsics

[Philip Reames via llvm-commits]

reames added a subscriber: reames.
reames added a comment.

Minor comments on the patch inline.

Elena - This seems like a necessary enhancement to support non-zero address spaces - which is a documented and supported part of the IR.  I understand your concern about readability, but I think that's outweighed by correctness.  Do you have any suggestions on how to improve this patch while meeting it's objective?


================
Comment at: docs/LangRef.rst:11336
@@ -11335,3 +11335,3 @@
 
-      declare <16 x float>  @llvm.masked.load.v16f32 (<16 x float>* <ptr>, i32 <alignment>, <16 x i1> <mask>, <16 x float> <passthru>)
-      declare <2 x double>  @llvm.masked.load.v2f64  (<2 x double>* <ptr>, i32 <alignment>, <2 x i1>  <mask>, <2 x double> <passthru>)
+      declare <16 x float>  @llvm.masked.load.v16f32.p0v16f32 (<16 x float>* <ptr>, i32 <alignment>, <16 x i1> <mask>, <16 x float> <passthru>)
+      declare <2 x double>  @llvm.masked.load.v2f64.p0v2f64  (<2 x double>* <ptr>, i32 <alignment>, <2 x i1>  <mask>, <2 x double> <passthru>)
----------------
Hm, looking at this mangling, the later bit implies the former.  Is there a way to rewrite the definition such that the vector type is inferred from the pointer type?

(The code is correct, just slightly verbose.)

================
Comment at: include/llvm/IR/IRBuilder.h:515
@@ -514,3 +514,3 @@
   /// \brief Create a call to a masked intrinsic with given Id.
-  /// Masked intrinsic has only one overloaded type - data type.
   CallInst *CreateMaskedIntrinsic(Intrinsic::ID Id, ArrayRef<Value *> Ops,
----------------
Would be good to clarify that the data type is implied by the pointer type and not passed explicitly.

================
Comment at: lib/IR/IRBuilder.cpp:218
@@ -217,3 +217,1 @@
-  Type *DataTy = cast<PointerType>(Ptr->getType())->getElementType();
-  assert(DataTy->isVectorTy() && "Ptr should point to a vector");
   if (!PassThru)
----------------
You should keep this assert.


http://reviews.llvm.org/D17270