Page MenuHomePhabricator

[InstCombine] canonicalize fmin/fmax to LLVM intrinsics minnum/maxnum
ClosedPublic

Authored by spatel on Jun 12 2019, 9:02 AM.

Details

Summary

This transform came up in D62414, but we should deal with it first.
We have LLVM intrinsics that correspond exactly to libm calls (unlike most libm calls, these libm calls never set errno).
This holds without any fast-math-flags, so we should always canonicalize to those intrinsics directly for better optimization.
Currently, we convert to fcmp+select only when we have FMF (nnan) because fcmp+select does not preserve the semantics of the call in the general case.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Jun 12 2019, 9:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 12 2019, 9:02 AM
This revision is now accepted and ready to land.Jun 12 2019, 11:36 AM
spatel updated this revision to Diff 205690.Wed, Jun 19, 3:39 PM

Patch updated:
Based on the discussion in D63294, we can't be too strict about matching libm calls in IR (a mismatched 'f' or 'l' suffix is ok), so I've updated the test that was checking for that case.

spatel requested review of this revision.Wed, Jun 19, 4:25 PM

This LGTM at a high level, but I don't fully understand the AVR concerns from D63294. Maybe @eli.friedman would be a better reviewer?

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1583 ↗(On Diff #205690)

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.

spatel marked an inline comment as done.Thu, Jun 27, 2:25 PM

This LGTM at a high level, but I don't fully understand the AVR concerns from 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).

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1583 ↗(On Diff #205690)

I'm fine with not including nsz here, but I think we should update the LangRef along with that change as a follow-up?

This revision is now accepted and ready to land.Thu, Jun 27, 3:33 PM
This revision was automatically updated to reflect the committed changes.