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

[Cameron McInally via Phabricator via llvm-commits]

cameron.mcinally accepted this revision.
cameron.mcinally marked an inline comment as done.
cameron.mcinally added a comment.
This revision is now accepted and ready to land.

LGTM



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


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

https://reviews.llvm.org/D62414