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

[Cameron McInally via Phabricator via llvm-commits]

cameron.mcinally 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]]
----------------
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.



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

https://reviews.llvm.org/D62414