<div dir="ltr"><div>Thanks for catching that.</div><div><br></div><div>I forgot about the PPC_FP128 type, and I'm not sure what it means to bitcast that type to an integer. The safe thing will be to just exclude that type from the transform. There's precedence for that kind of restriction in other places in LLVM. <br></div><div><br></div><div>AFAIK, that's the only problematic FP type in LLVM [1]. But let me know if you see other issues. I'll post a patch for review shortly. <br></div><div><br></div><div>Side note - Alive2 says that ppc_fp128 is an unsupported type, so we can't use it to say anything about this example:<br></div><div>define i1 @src(float %x) {<br> %s2 = fpext float %x to ppc_fp128<br> %s3 = bitcast ppc_fp128 %s2 to i128<br> %s4 = icmp slt i128 %s3, 0<br> ret i1 %s4<br>}<br></div><div><br></div><div>[1] <a href="http://llvm.org/docs/LangRef.html#floating-point-types">http://llvm.org/docs/LangRef.html#floating-point-types</a></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Apr 6, 2020 at 6:53 PM Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Is this going to handle -infinity correctly on some (power) platforms?<div><br></div><div>When hashing <span style="color:rgb(32,33,36);font-family:Roboto,Arial,sans-serif;font-size:13px;letter-spacing:0.185714px;white-space:pre-wrap">-std::numeric_limits<long double>::infinity() we get:</span></div><div><br></div><div><span style="color:rgb(32,33,36);font-family:Roboto,Arial,sans-serif;font-size:13px;letter-spacing:0.185714px;white-space:pre-wrap">good version:
0x0000: 01
0x0000: 0100 0000
bad version:
0x0000: 00
0x0000: 0100 0000
<br></span></div><div><span style="color:rgb(32,33,36);font-family:Roboto,Arial,sans-serif;font-size:13px;letter-spacing:0.185714px;white-space:pre-wrap">which has exactly the sign bit of difference and generates the same hash value as </span><span style="color:rgb(32,33,36);font-family:Roboto,Arial,sans-serif;font-size:13px;letter-spacing:0.185714px;white-space:pre-wrap">std::numeric_limits<long double>::infinity(). Possibly a bug in the hasher here, but wanted to bring it up.</span><span style="color:rgb(32,33,36);font-family:Roboto,Arial,sans-serif;font-size:13px;letter-spacing:0.185714px;white-space:pre-wrap">
</span></div><div><span style="color:rgb(32,33,36);font-family:Roboto,Arial,sans-serif;font-size:13px;letter-spacing:0.185714px;white-space:pre-wrap"><br></span></div><div><font face="Roboto, Arial, sans-serif" color="#202124"><span style="letter-spacing:0.185714px;white-space:pre-wrap">Thoughts?</span></font></div><div><font face="Roboto, Arial, sans-serif" color="#202124"><span style="letter-spacing:0.185714px;white-space:pre-wrap"><br></span></font></div><div><font face="Roboto, Arial, sans-serif" color="#202124"><span style="letter-spacing:0.185714px;white-space:pre-wrap">-eric</span></font></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Mar 28, 2020 at 6:47 AM Sanjay Patel via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>Alive2 confirms the -0.0 problem:</div><div><a href="http://volta.cs.utah.edu:8080/z/f39Rw-" target="_blank">http://volta.cs.utah.edu:8080/z/f39Rw-</a></div><div><br></div><div>NaN inputs could also be wrong. We could do better given fast-math-flags, but I'm avoiding that without strong motivation...because FMF on fcmp is a mess ( <a href="https://bugs.llvm.org/show_bug.cgi?id=38086" target="_blank">https://bugs.llvm.org/show_bug.cgi?id=38086</a> ).<br></div><div>(It would be interesting to have Alive2 command-line options to disallow -0.0 or NaN values similar to the -disable-undef-input option.)<br></div><div><br></div><div>We do have this transform for fcmp already:</div><div> // fcmp (fpext X), C -> fcmp X, (fptrunc C) if fptrunc is lossless</div><div><br></div><div>And I think that's as far as we can take it - there is no fptrunc sibling for that because truncation loses bits:</div><div></div><div><a href="http://volta.cs.utah.edu:8080/z/zTheyA" target="_blank">http://volta.cs.utah.edu:8080/z/zTheyA</a></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Mar 28, 2020 at 4:38 AM Roman Lebedev <<a href="mailto:lebedev.ri@gmail.com" target="_blank">lebedev.ri@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Ah yes, indeed. Sorry for the noise.<br>
<br>
On Sat, Mar 28, 2020 at 10:31 AM Craig Topper <<a href="mailto:craig.topper@gmail.com" target="_blank">craig.topper@gmail.com</a>> wrote:<br>
><br>
> Fp compare wouldn’t handle negative 0 correctly would it?<br>
><br>
> On Fri, Mar 27, 2020 at 11:27 PM Roman Lebedev via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>> On Sat, Mar 28, 2020 at 12:34 AM Sanjay Patel via llvm-commits<br>
>> <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
>> ><br>
>> ><br>
>> > Author: Sanjay Patel<br>
>> > Date: 2020-03-27T17:33:59-04:00<br>
>> > New Revision: 0f56bbc1a5b2ddc881d1c55c9024b9c473dac6f0<br>
>> ><br>
>> > URL: <a href="https://github.com/llvm/llvm-project/commit/0f56bbc1a5b2ddc881d1c55c9024b9c473dac6f0" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/0f56bbc1a5b2ddc881d1c55c9024b9c473dac6f0</a><br>
>> > DIFF: <a href="https://github.com/llvm/llvm-project/commit/0f56bbc1a5b2ddc881d1c55c9024b9c473dac6f0.diff" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/0f56bbc1a5b2ddc881d1c55c9024b9c473dac6f0.diff</a><br>
>> ><br>
>> > LOG: [InstCombine] reduce FP-casted and bitcasted signbit check<br>
>> ><br>
>> > PR45305:<br>
>> > <a href="https://bugs.llvm.org/show_bug.cgi?id=45305" rel="noreferrer" target="_blank">https://bugs.llvm.org/show_bug.cgi?id=45305</a><br>
>> ><br>
>> > Alive2 proofs:<br>
>> > <a href="http://volta.cs.utah.edu:8080/z/bVyrko" rel="noreferrer" target="_blank">http://volta.cs.utah.edu:8080/z/bVyrko</a><br>
>> > <a href="http://volta.cs.utah.edu:8080/z/Vxpz9q" rel="noreferrer" target="_blank">http://volta.cs.utah.edu:8080/z/Vxpz9q</a><br>
>> ><br>
>> > Added:<br>
>> ><br>
>> ><br>
>> > Modified:<br>
>> > llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp<br>
>> > llvm/test/Transforms/InstCombine/icmp.ll<br>
>> ><br>
>> > Removed:<br>
>> ><br>
>> ><br>
>> ><br>
>> > ################################################################################<br>
>> > diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp<br>
>> > index 63bf3462faac..62b99e8b8b5b 100644<br>
>> > --- a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp<br>
>> > +++ b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp<br>
>> > @@ -2760,6 +2760,30 @@ static Instruction *foldICmpBitCast(ICmpInst &Cmp,<br>
>> > if (match(BCSrcOp, m_UIToFP(m_Value(X))))<br>
>> > if (Cmp.isEquality() && match(Op1, m_Zero()))<br>
>> > return new ICmpInst(Pred, X, ConstantInt::getNullValue(X->getType()));<br>
>> > +<br>
>> > + // If this is a sign-bit test of a bitcast of a casted FP value, eliminate<br>
>> > + // the FP cast because the FP cast does not change the sign-bit.<br>
>> > + const APInt *C;<br>
>> > + bool TrueIfSigned;<br>
>> > + if (match(Op1, m_APInt(C)) && Bitcast->hasOneUse() &&<br>
>> > + isSignBitCheck(Pred, *C, TrueIfSigned)) {<br>
>> > + if (match(BCSrcOp, m_FPExt(m_Value(X))) ||<br>
>> > + match(BCSrcOp, m_FPTrunc(m_Value(X)))) {<br>
>> > + // (bitcast (fpext/fptrunc X)) to iX) < 0 --> (bitcast X to iY) < 0<br>
>> > + // (bitcast (fpext/fptrunc X)) to iX) > -1 --> (bitcast X to iY) > -1<br>
>> > + Type *XType = X->getType();<br>
>> > + Type *NewType = Builder.getIntNTy(XType->getScalarSizeInBits());<br>
>> > + if (XType->isVectorTy())<br>
>> > + NewType = VectorType::get(NewType, XType->getVectorNumElements());<br>
>> > + Value *NewBitcast = Builder.CreateBitCast(X, NewType);<br>
>> > + if (TrueIfSigned)<br>
>> > + return new ICmpInst(ICmpInst::ICMP_SLT, NewBitcast,<br>
>> > + ConstantInt::getNullValue(NewType));<br>
>> > + else<br>
>> > + return new ICmpInst(ICmpInst::ICMP_SGT, NewBitcast,<br>
>> > + ConstantInt::getAllOnesValue(NewType));<br>
>> > + }<br>
>> > + }<br>
>> > }<br>
>> ><br>
>> > // Test to see if the operands of the icmp are casted versions of other<br>
>> ><br>
>> > diff --git a/llvm/test/Transforms/InstCombine/icmp.ll b/llvm/test/Transforms/InstCombine/icmp.ll<br>
>> > index b2c6213399b0..41bf661b092e 100644<br>
>> > --- a/llvm/test/Transforms/InstCombine/icmp.ll<br>
>> > +++ b/llvm/test/Transforms/InstCombine/icmp.ll<br>
>> > @@ -3609,9 +3609,8 @@ define <2 x i32> @Op1Negated_Vec(<2 x i32> %x) {<br>
>> ><br>
>> > define i1 @signbit_bitcast_fpext(float %x) {<br>
>> > ; CHECK-LABEL: @signbit_bitcast_fpext(<br>
>> > -; CHECK-NEXT: [[F:%.*]] = fpext float [[X:%.*]] to double<br>
>> > -; CHECK-NEXT: [[B:%.*]] = bitcast double [[F]] to i64<br>
>> > -; CHECK-NEXT: [[R:%.*]] = icmp slt i64 [[B]], 0<br>
>> > +; CHECK-NEXT: [[TMP1:%.*]] = bitcast float [[X:%.*]] to i32<br>
>> > +; CHECK-NEXT: [[R:%.*]] = icmp slt i32 [[TMP1]], 0<br>
>> > ; CHECK-NEXT: ret i1 [[R]]<br>
>> Not that i disagree with the transform, but why do we want to keep bitcast<br>
>> instead of producing fp comparison?<br>
>><br>
>> We then would want to split this fold:<br>
>> (bitcast (fpext/fptrunc X)) to iX) < 0 --> (fpext/fptrunc X) < 0<br>
>> (fpext/fptrunc X) < 0 --> X < 0<br>
>><br>
>> > %f = fpext float %x to double<br>
>> > @@ -3622,9 +3621,8 @@ define i1 @signbit_bitcast_fpext(float %x) {<br>
>> ><br>
>> > define <2 x i1> @signbit_bitcast_fpext_vec(<2 x half> %x) {<br>
>> > ; CHECK-LABEL: @signbit_bitcast_fpext_vec(<br>
>> > -; CHECK-NEXT: [[F:%.*]] = fpext <2 x half> [[X:%.*]] to <2 x float><br>
>> > -; CHECK-NEXT: [[B:%.*]] = bitcast <2 x float> [[F]] to <2 x i32><br>
>> > -; CHECK-NEXT: [[R:%.*]] = icmp slt <2 x i32> [[B]], zeroinitializer<br>
>> > +; CHECK-NEXT: [[TMP1:%.*]] = bitcast <2 x half> [[X:%.*]] to <2 x i16><br>
>> > +; CHECK-NEXT: [[R:%.*]] = icmp slt <2 x i16> [[TMP1]], zeroinitializer<br>
>> > ; CHECK-NEXT: ret <2 x i1> [[R]]<br>
>> > ;<br>
>> > %f = fpext <2 x half> %x to <2 x float><br>
>> > @@ -3635,9 +3633,8 @@ define <2 x i1> @signbit_bitcast_fpext_vec(<2 x half> %x) {<br>
>> ><br>
>> > define i1 @signbit_bitcast_fptrunc(float %x) {<br>
>> > ; CHECK-LABEL: @signbit_bitcast_fptrunc(<br>
>> > -; CHECK-NEXT: [[F:%.*]] = fptrunc float [[X:%.*]] to half<br>
>> > -; CHECK-NEXT: [[B:%.*]] = bitcast half [[F]] to i16<br>
>> > -; CHECK-NEXT: [[R:%.*]] = icmp sgt i16 [[B]], -1<br>
>> > +; CHECK-NEXT: [[TMP1:%.*]] = bitcast float [[X:%.*]] to i32<br>
>> > +; CHECK-NEXT: [[R:%.*]] = icmp sgt i32 [[TMP1]], -1<br>
>> > ; CHECK-NEXT: ret i1 [[R]]<br>
>> > ;<br>
>> > %f = fptrunc float %x to half<br>
>> > @@ -3648,9 +3645,8 @@ define i1 @signbit_bitcast_fptrunc(float %x) {<br>
>> ><br>
>> > define <2 x i1> @signbit_bitcast_fptrunc_vec(<2 x double> %x) {<br>
>> > ; CHECK-LABEL: @signbit_bitcast_fptrunc_vec(<br>
>> > -; CHECK-NEXT: [[F:%.*]] = fptrunc <2 x double> [[X:%.*]] to <2 x half><br>
>> > -; CHECK-NEXT: [[B:%.*]] = bitcast <2 x half> [[F]] to <2 x i16><br>
>> > -; CHECK-NEXT: [[R:%.*]] = icmp sgt <2 x i16> [[B]], <i16 -1, i16 -1><br>
>> > +; CHECK-NEXT: [[TMP1:%.*]] = bitcast <2 x double> [[X:%.*]] to <2 x i64><br>
>> > +; CHECK-NEXT: [[R:%.*]] = icmp sgt <2 x i64> [[TMP1]], <i64 -1, i64 -1><br>
>> > ; CHECK-NEXT: ret <2 x i1> [[R]]<br>
>> > ;<br>
>> > %f = fptrunc <2 x double> %x to <2 x half><br>
>> > @@ -3672,6 +3668,19 @@ define i1 @signbit_bitcast_fpext_wrong_cmp(float %x) {<br>
>> > ret i1 %r<br>
>> > }<br>
>> ><br>
>> > +define <4 x i1> @signbit_bitcast_fpext_vec_wrong_bitcast(<2 x half> %x) {<br>
>> > +; CHECK-LABEL: @signbit_bitcast_fpext_vec_wrong_bitcast(<br>
>> > +; CHECK-NEXT: [[F:%.*]] = fpext <2 x half> [[X:%.*]] to <2 x float><br>
>> > +; CHECK-NEXT: [[B:%.*]] = bitcast <2 x float> [[F]] to <4 x i16><br>
>> > +; CHECK-NEXT: [[R:%.*]] = icmp sgt <4 x i16> [[B]], <i16 -1, i16 -1, i16 -1, i16 -1><br>
>> > +; CHECK-NEXT: ret <4 x i1> [[R]]<br>
>> > +;<br>
>> > + %f = fpext <2 x half> %x to <2 x float><br>
>> > + %b = bitcast <2 x float> %f to <4 x i16><br>
>> > + %r = icmp sgt <4 x i16> %b, <i16 -1, i16 -1, i16 -1, i16 -1><br>
>> > + ret <4 x i1> %r<br>
>> > +}<br>
>> > +<br>
>> > declare void @use_i64(i64)<br>
>> ><br>
>> > define i1 @signbit_bitcast_fpext_extra_use(float %x, i64* %p) {<br>
>> ><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="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><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="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
><br>
> --<br>
> ~Craig<br>
</blockquote></div>
_______________________________________________<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="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>
</blockquote></div>