[PATCH] D17270: Support arbitrary addrspace pointers in masked load/store intrinsics
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 16 14:47:52 PST 2016
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
More information about the llvm-commits
mailing list