[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:
> > > 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.


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

https://reviews.llvm.org/D62414