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

[Michael Kuperstein via llvm-commits]

mkuper added inline comments.

================
Comment at: ../lib/CodeGen/CodeGenPrepare.cpp:1361
@@ +1360,3 @@
+                                                "Ptr" + Twine(Idx));
+      LoadInst* Load = Builder.CreateAlignedLoad(Ptr, AlignVal,
+                                                 "Load" + Twine(Idx));
----------------
delena wrote:
> mkuper wrote:
> > Don't you need AlignVal = std::min(AlignVal, VecType->getScalarSizeInBits()), like for the load/store case?
> No. Gather / scatter alignment means alignment for one element, not for a vector. If the specified alignment is bigger than element size, we can't handle it properly on X86. The only one option that I see here is to scalarize masked-gather-scatter to a chain of scalar loads with required alignment.
Ah, right, of course.

================
Comment at: ../lib/Target/X86/X86TargetTransformInfo.cpp:1212
@@ +1211,3 @@
+}
+
+bool X86TTIImpl::isLegalMaskedScatter(Type *DataType) {
----------------
delena wrote:
> mkuper wrote:
> > 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.
> a) I can add rejection of the not-power-of-2 cases. And a test.
> But I want to know your opinion about vector and scalar types in this function:
> (This comment I want to put inside)
>   // This function is called now in two cases: from the Loop Vectorizer
>   // and from the Scalarizer.
>   // When the Loop Vectorizer asks about legality of the feature,
>   // the vectorization factor is not calculated yet. The Loop Vectorizer
>   // sends a scalar type and the decision is based on the width of the
>   // scalar element.
>   // Later on, the cost model will estimate usage this intrinsic based on
>   // the vector type.
>   // The Scalarizer asks again about legality. It sends a vector type.
>   // In this case we can reject non-power-of-2 vectors.
>   if (isa<VectorType>(DataTy) && !isPowerOf2_32(DataTy->getVectorNumElements()))
>     return false;
>   Type *ScalarTy = DataTy->getScalarType();
>    ...
> b) "IsLegal" comes from TypeLegalizer and it is equal to "IsSupported". I wrote in the comments in  TargetTransformInfo.h "Return true if the target supports.."
SGTM.


Repository:
  rL LLVM

http://reviews.llvm.org/D13722