[PATCH] D62414: [InstCombine] canonicalize fcmp+select to minnum/maxnum intrinsics

[Sanjay Patel via Phabricator via llvm-commits]

spatel marked an inline comment as done.
spatel added inline comments.


================
Comment at: llvm/test/Transforms/InstCombine/fast-math.ll:884
 ; CHECK-LABEL: @min3(
-; CHECK-NEXT:    [[TMP1:%.*]] = fcmp nnan nsz olt double [[A:%.*]], [[B:%.*]]
-; CHECK-NEXT:    [[TMP2:%.*]] = select nnan nsz i1 [[TMP1]], double [[A]], double [[B]]
-; CHECK-NEXT:    ret double [[TMP2]]
+; CHECK-NEXT:    [[TMP1:%.*]] = call nnan nsz double @llvm.minnum.f64(double [[A:%.*]], double [[B:%.*]])
+; CHECK-NEXT:    ret double [[TMP1]]
----------------
cameron.mcinally wrote:
> spatel wrote:
> > cameron.mcinally wrote:
> > > spatel wrote:
> > > > cameron.mcinally wrote:
> > > > > cameron.mcinally wrote:
> > > > > > Do we want NSZ here? I see that this was an artifact of the old fcmp+select lowering, but I'm not sure if it is correct. Definitely not IEEE-754 compliant...
> > > > > Now that I think about it, maybe it would be better to skip the fcmp+sel step and lower the libm call directly into the intrinsic?
> > > > Can you explain/show example for the compliancy problem?
> > > > 
> > > > Reference for our current docs:
> > > > http://llvm.org/docs/LangRef.html#llvm-minnum-intrinsic
> > > Ah, right. This is ok in IEEE-754 2008. My copy of IEEE-754 2018 (a draft):
> > > 
> > > >minimumNumber(x, y) is x if x < y, y if y < x, and the number if one operand is a number and the 25 other is a NaN. !!!For this operation, −0 compares less than +0.!!! If x = y and signs are the same it is either x or y. If both operands are NaNs, a quiet NaN is returned, according to 6.2. If either operand is a signaling NaN, an invalid operation exception is signaled, but unless both operands are NaNs, the signaling NaN is otherwise ignored and not converted to a quiet NaN as stated in 6.2 for other operations.
> > > 
> > > So adding NSZ there wouldn't be correct for the min(-0,+0) and friends cases.
> > > 
> > I might still be missing some subtlety. 
> > 
> > My interpretation of the LangRef text (and so also IEEE-754 2008) is that 'nsz' ("Allow optimizations to treat the sign of a zero argument or result as insignificant") is implicit in the definition of minnum/maxnum.
> > 
> > By explicitly propagating the 'nsz' in this case, we are future-proofing the behavior even for the new standard. The original fcmp+select said that sign of zero is a 'don't care', and we want the intrinsic call to have that same freedom.
> Yeah, I agree your new code is fine.
> 
> I was questioning whether we should be adding NSZ during the @fmin(...) lowering, since the NSZ isn't on the original fmin(...) call. But now I don't see anything that requires @fmin*(...) to honor the sign of a zero in the Standards or libm. So this is fine too.
> 
> Sorry for the noise...
Ah, I see now. Not noise at all - these are excellent and valid points.

It goes back to your other comment - we could be transforming the fmin call directly to the intrinsic in these examples. So that confusion was just caused by me being lazy and not creating minimal tests for this patch. Let me do that, then I'll try to fix up that existing transform.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62414/new/

https://reviews.llvm.org/D62414