<div dir="ltr">Yes, I misread the manual. Working on the fix now. Thanks for letting me know!<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Apr 12, 2016 at 8:49 AM, Andrea Di Biagio <span dir="ltr"><<a href="mailto:andrea.dibiagio@gmail.com" target="_blank">andrea.dibiagio@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div><div>Hi Sanjay,<br><br></div>We noticed a regression after this commit which seems to be caused by an incorrect folding of a x86 masked loads intrinsic with a zero mask.<br></div><br></div>According to the Intel documentation:<br>"<br>The <var>mask</var> is calculated from the most significant bit of each dword of the <var>mask</var> register. If any of the bits of the <var>mask</var>
is set to zero, the corresponding value from the memory location is not
loaded, and the corresponding field of the destination vector is set to
zero.<br>"<br>
<div><div><br></div><div>I suspect that the problem is caused by the following incorrect check:<span class=""><br><br>+ // Special case a zero mask since that's not a ConstantDataVector.<br>
+ // This masked load instruction does nothing, so return an undef.<br>
+ if (isa<ConstantAggregateZero>(Mask))<br>
+ return IC.replaceInstUsesWith(II, UndefValue::get(II.getType()));<br>
+<br><br></span></div><div>The behavior of a avx masked load intrinsic with a zero mask is very well defined. Instead of propagating Undef, you should have propagated a zero vector.<br><br></div><div>Can you please have a look at it?<br><br></div><div>Thanks in advance,<br></div><div>-Andrea<br></div></div></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Feb 29, 2016 at 11:16 PM, Sanjay Patel via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: spatel<br>
Date: Mon Feb 29 17:16:48 2016<br>
New Revision: 262269<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=262269&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=262269&view=rev</a><br>
Log:<br>
[x86, InstCombine] transform x86 AVX masked loads to LLVM intrinsics<br>
<br>
The intended effect of this patch in conjunction with:<br>
<a href="http://reviews.llvm.org/rL259392" rel="noreferrer" target="_blank">http://reviews.llvm.org/rL259392</a><br>
<a href="http://reviews.llvm.org/rL260145" rel="noreferrer" target="_blank">http://reviews.llvm.org/rL260145</a><br>
<br>
is that customers using the AVX intrinsics in C will benefit from combines when<br>
the load mask is constant:<br>
<br>
__m128 mload_zeros(float *f) {<br>
return _mm_maskload_ps(f, _mm_set1_epi32(0));<br>
}<br>
<br>
__m128 mload_fakeones(float *f) {<br>
return _mm_maskload_ps(f, _mm_set1_epi32(1));<br>
}<br>
<br>
__m128 mload_ones(float *f) {<br>
return _mm_maskload_ps(f, _mm_set1_epi32(0x80000000));<br>
}<br>
<br>
__m128 mload_oneset(float *f) {<br>
return _mm_maskload_ps(f, _mm_set_epi32(0x80000000, 0, 0, 0));<br>
}<br>
<br>
...so none of the above will actually generate a masked load for optimized code.<br>
<br>
This is the masked load counterpart to:<br>
<a href="http://reviews.llvm.org/rL262064" rel="noreferrer" target="_blank">http://reviews.llvm.org/rL262064</a><br>
<br>
<br>
Modified:<br>
llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp<br>
llvm/trunk/test/Transforms/InstCombine/x86-masked-memops.ll<br>
<br>
Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp?rev=262269&r1=262268&r2=262269&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp?rev=262269&r1=262268&r2=262269&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp Mon Feb 29 17:16:48 2016<br>
@@ -831,6 +831,39 @@ static Instruction *simplifyMaskedScatte<br>
// TODO: If the x86 backend knew how to convert a bool vector mask back to an<br>
// XMM register mask efficiently, we could transform all x86 masked intrinsics<br>
// to LLVM masked intrinsics and remove the x86 masked intrinsic defs.<br>
+static Instruction *simplifyX86MaskedLoad(IntrinsicInst &II, InstCombiner &IC) {<br>
+ Value *Ptr = II.getOperand(0);<br>
+ Value *Mask = II.getOperand(1);<br>
+<br>
+ // Special case a zero mask since that's not a ConstantDataVector.<br>
+ // This masked load instruction does nothing, so return an undef.<br>
+ if (isa<ConstantAggregateZero>(Mask))<br>
+ return IC.replaceInstUsesWith(II, UndefValue::get(II.getType()));<br>
+<br>
+ auto *ConstMask = dyn_cast<ConstantDataVector>(Mask);<br>
+ if (!ConstMask)<br>
+ return nullptr;<br>
+<br>
+ // The mask is constant. Convert this x86 intrinsic to the LLVM instrinsic<br>
+ // to allow target-independent optimizations.<br>
+<br>
+ // First, cast the x86 intrinsic scalar pointer to a vector pointer to match<br>
+ // the LLVM intrinsic definition for the pointer argument.<br>
+ unsigned AddrSpace = cast<PointerType>(Ptr->getType())->getAddressSpace();<br>
+ PointerType *VecPtrTy = PointerType::get(II.getType(), AddrSpace);<br>
+ Value *PtrCast = IC.Builder->CreateBitCast(Ptr, VecPtrTy, "castvec");<br>
+<br>
+ // Second, convert the x86 XMM integer vector mask to a vector of bools based<br>
+ // on each element's most significant bit (the sign bit).<br>
+ Constant *BoolMask = getNegativeIsTrueBoolVec(ConstMask);<br>
+<br>
+ CallInst *NewMaskedLoad = IC.Builder->CreateMaskedLoad(PtrCast, 1, BoolMask);<br>
+ return IC.replaceInstUsesWith(II, NewMaskedLoad);<br>
+}<br>
+<br>
+// TODO: If the x86 backend knew how to convert a bool vector mask back to an<br>
+// XMM register mask efficiently, we could transform all x86 masked intrinsics<br>
+// to LLVM masked intrinsics and remove the x86 masked intrinsic defs.<br>
static bool simplifyX86MaskedStore(IntrinsicInst &II, InstCombiner &IC) {<br>
Value *Ptr = II.getOperand(0);<br>
Value *Mask = II.getOperand(1);<br>
@@ -854,7 +887,6 @@ static bool simplifyX86MaskedStore(Intri<br>
// the LLVM intrinsic definition for the pointer argument.<br>
unsigned AddrSpace = cast<PointerType>(Ptr->getType())->getAddressSpace();<br>
PointerType *VecPtrTy = PointerType::get(Vec->getType(), AddrSpace);<br>
-<br>
Value *PtrCast = IC.Builder->CreateBitCast(Ptr, VecPtrTy, "castvec");<br>
<br>
// Second, convert the x86 XMM integer vector mask to a vector of bools based<br>
@@ -1630,6 +1662,12 @@ Instruction *InstCombiner::visitCallInst<br>
return replaceInstUsesWith(*II, V);<br>
break;<br>
<br>
+ case Intrinsic::x86_avx_maskload_ps:<br>
+ // TODO: Add the other masked load variants.<br>
+ if (Instruction *I = simplifyX86MaskedLoad(*II, *this))<br>
+ return I;<br>
+ break;<br>
+<br>
case Intrinsic::x86_avx_maskstore_ps:<br>
case Intrinsic::x86_avx_maskstore_pd:<br>
case Intrinsic::x86_avx_maskstore_ps_256:<br>
<br>
Modified: llvm/trunk/test/Transforms/InstCombine/x86-masked-memops.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/x86-masked-memops.ll?rev=262269&r1=262268&r2=262269&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/x86-masked-memops.ll?rev=262269&r1=262268&r2=262269&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/Transforms/InstCombine/x86-masked-memops.ll (original)<br>
+++ llvm/trunk/test/Transforms/InstCombine/x86-masked-memops.ll Mon Feb 29 17:16:48 2016<br>
@@ -1,5 +1,64 @@<br>
; RUN: opt < %s -instcombine -S | FileCheck %s<br>
<br>
+;; MASKED LOADS<br>
+<br>
+; If the mask isn't constant, do nothing.<br>
+<br>
+define <4 x float> @mload(i8* %f, <4 x i32> %mask) {<br>
+ %ld = tail call <4 x float> @<a href="http://llvm.x86.avx.maskload.ps" rel="noreferrer" target="_blank">llvm.x86.avx.maskload.ps</a>(i8* %f, <4 x i32> %mask)<br>
+ ret <4 x float> %ld<br>
+<br>
+; CHECK-LABEL: @mload(<br>
+; CHECK-NEXT: %ld = tail call <4 x float> @<a href="http://llvm.x86.avx.maskload.ps" rel="noreferrer" target="_blank">llvm.x86.avx.maskload.ps</a>(i8* %f, <4 x i32> %mask)<br>
+; CHECK-NEXT: ret <4 x float> %ld<br>
+}<br>
+<br>
+; Zero mask is a nop.<br>
+<br>
+define <4 x float> @mload_zeros(i8* %f) {<br>
+ %ld = tail call <4 x float> @<a href="http://llvm.x86.avx.maskload.ps" rel="noreferrer" target="_blank">llvm.x86.avx.maskload.ps</a>(i8* %f, <4 x i32> zeroinitializer)<br>
+ ret <4 x float> %ld<br>
+<br>
+; CHECK-LABEL: @mload_zeros(<br>
+; CHECK-NEXT: ret <4 x float> undef<br>
+}<br>
+<br>
+; Only the sign bit matters.<br>
+<br>
+define <4 x float> @mload_fake_ones(i8* %f) {<br>
+ %ld = tail call <4 x float> @<a href="http://llvm.x86.avx.maskload.ps" rel="noreferrer" target="_blank">llvm.x86.avx.maskload.ps</a>(i8* %f, <4 x i32> <i32 1, i32 2, i32 3, i32 2147483647>)<br>
+ ret <4 x float> %ld<br>
+<br>
+; CHECK-LABEL: @mload_fake_ones(<br>
+; CHECK-NEXT: ret <4 x float> undef<br>
+}<br>
+<br>
+; All mask bits are set, so this is just a vector load.<br>
+<br>
+define <4 x float> @mload_real_ones(i8* %f) {<br>
+ %ld = tail call <4 x float> @<a href="http://llvm.x86.avx.maskload.ps" rel="noreferrer" target="_blank">llvm.x86.avx.maskload.ps</a>(i8* %f, <4 x i32> <i32 -1, i32 -2, i32 -3, i32 2147483648>)<br>
+ ret <4 x float> %ld<br>
+<br>
+; CHECK-LABEL: @mload_real_ones(<br>
+; CHECK-NEXT: %castvec = bitcast i8* %f to <4 x float>*<br>
+; CHECK-NEXT: %unmaskedload = load <4 x float>, <4 x float>* %castvec<br>
+; CHECK-NEXT: ret <4 x float> %unmaskedload<br>
+}<br>
+<br>
+; It's a constant mask, so convert to an LLVM intrinsic. The backend should optimize further.<br>
+<br>
+define <4 x float> @mload_one_one(i8* %f) {<br>
+ %ld = tail call <4 x float> @<a href="http://llvm.x86.avx.maskload.ps" rel="noreferrer" target="_blank">llvm.x86.avx.maskload.ps</a>(i8* %f, <4 x i32> <i32 0, i32 0, i32 0, i32 -1>)<br>
+ ret <4 x float> %ld<br>
+<br>
+; CHECK-LABEL: @mload_one_one(<br>
+; CHECK-NEXT: %castvec = bitcast i8* %f to <4 x float>*<br>
+; CHECK-NEXT: %1 = call <4 x float> @llvm.masked.load.v4f32(<4 x float>* %castvec, i32 1, <4 x i1> <i1 false, i1 false, i1 false, i1 true>, <4 x float> undef)<br>
+; CHECK-NEXT: ret <4 x float> %1<br>
+}<br>
+<br>
+;; MASKED STORES<br>
+<br>
; If the mask isn't constant, do nothing.<br>
<br>
define void @mstore(i8* %f, <4 x i32> %mask, <4 x float> %v) {<br>
@@ -131,6 +190,10 @@ define void @mstore_v4i64(i8* %f, <4 x i<br>
; CHECK-NEXT: ret void<br>
}<br>
<br>
+declare <4 x float> @<a href="http://llvm.x86.avx.maskload.ps" rel="noreferrer" target="_blank">llvm.x86.avx.maskload.ps</a>(i8*, <4 x i32>)<br>
+declare <2 x double> @llvm.x86.avx.maskload.pd(i8*, <2 x i64>)<br>
+declare <8 x float> @llvm.x86.avx.maskload.ps.256(i8*, <8 x i32>)<br>
+declare <4 x double> @llvm.x86.avx.maskload.pd.256(i8*, <4 x i64>)<br>
<br>
declare void @<a href="http://llvm.x86.avx.maskstore.ps" rel="noreferrer" target="_blank">llvm.x86.avx.maskstore.ps</a>(i8*, <4 x i32>, <4 x float>)<br>
declare void @llvm.x86.avx.maskstore.pd(i8*, <2 x i64>, <2 x double>)<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>