[PATCH] D63214: [InstCombine] canonicalize fmin/fmax to LLVM intrinsics minnum/maxnum

Thu Jun 27 14:28:25 PDT 2019

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

In D63214#1561143 <https://reviews.llvm.org/D63214#1561143>, @cameron.mcinally wrote:

> This LGTM at a high level, but I don't fully understand the AVR concerns from D63294 <https://reviews.llvm.org/D63294>. Maybe @eli.friedman would be a better reviewer?

Yes - hoping that @efriedma will take a look when possible. As I understand it, FP types may be altered between C and LLVM, so what we think of as "float" or "double" is not necessarily consistent between C and LLVM. So I adjusted the test '@fake_fmin' to reflect that. I think that test is actually a fluke though because we are not consistently enforcing the mapping between lib function suffix and type (ie, we are probably doing other transforms using the less restrictive check that any FP type with a matching libm function name is a real libm call).

================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:1583
+  FastMathFlags FMF = CI->getFastMathFlags();
+  FMF.setNoSignedZeros();
   B.setFastMathFlags(FMF);
----------------
cameron.mcinally wrote:
> It's pretty clear that the next IEEE-754 will respect zero sign bits for fmin/fmax. Would there be a big difference now if we didn't set nsz here? My thinking is that this line will become a bug when the new draft lands (and it's fairly hidden).
> 
> That said, if there is a behavior change by not setting the nsz flag, then we should probably just wait.
I'm fine with not including nsz here, but I think we should update the LangRef along with that change as a follow-up?

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

https://reviews.llvm.org/D63214