This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Combine nested min/max intrinsics with constants
ClosedPublic

Authored by volkan on Oct 26 2018, 12:05 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

volkan created this revision.Oct 26 2018, 12:05 PM
  1. Please add the tests to trunk as a preliminary commit.
  2. Add tests with 0.0 and -0.0 constants, so we are sure that edge case is handled correctly.
  3. Sprinkle around some fast-math-flags, so we know those are propagated correctly.
  4. Make at least 1 test use vector splat constants, so we know that type works.

Side note: do you know if the reassociate pass handles these? I imagine we're going to miss the optimization in instcombine if the constants are separated in a longer chain of min/max ops.

lib/Transforms/InstCombine/InstCombineCalls.cpp
2067 ↗(On Diff #171329)

Just call this "M" to match the code comment.

volkan updated this revision to Diff 171735.Oct 30 2018, 11:15 AM
  • Updated the tests.
  • Added missing copyIRFlags call.
volkan marked an inline comment as done.Oct 30 2018, 11:17 AM

Side note: do you know if the reassociate pass handles these? I imagine we're going to miss the optimization in instcombine if the constants are separated in a longer chain of min/max ops.

Probably not, I couldn't find anything related to these intrinsics.

lebedev.ri added inline comments.
lib/Transforms/InstCombine/InstCombineCalls.cpp
2067–2072 ↗(On Diff #171735)

Can this be simplified at all using existing m_Intrinsic<>()?

volkan added inline comments.Oct 30 2018, 11:27 AM
lib/Transforms/InstCombine/InstCombineCalls.cpp
2067–2072 ↗(On Diff #171735)

m_Intrinsic<> can be used here, but that wouldn't be simpler as we need to call that for each intrinsics.

Is it worth adjusting/adding some tests with NaN constants?

test/Transforms/InstCombine/maxnum.ll
177 ↗(On Diff #171735)

maxnum(0.0, -0.0) is unspecified, but consistent within LLVM, right? Ie, we should not get random test failures on this test and the similar minnum test.

Is it worth adjusting/adding some tests with NaN constants?

I don't think it's necessary as there are already some tests in unittests/ADT/APFloatTest.cpp.

test/Transforms/InstCombine/maxnum.ll
177 ↗(On Diff #171735)

Yes, it's consistent since we use the functions defined in APFloat to get the result. See: maxnum in include/llvm/ADT/APFloat.h.

spatel accepted this revision.Oct 31 2018, 10:42 AM

LGTM

This revision is now accepted and ready to land.Oct 31 2018, 10:42 AM
This revision was automatically updated to reflect the committed changes.