Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
- Please add the tests to trunk as a preliminary commit.
- Add tests with 0.0 and -0.0 constants, so we are sure that edge case is handled correctly.
- Sprinkle around some fast-math-flags, so we know those are propagated correctly.
- 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. |
lib/Transforms/InstCombine/InstCombineCalls.cpp | ||
---|---|---|
2067–2072 ↗ | (On Diff #171735) | Can this be simplified at all using existing m_Intrinsic<>()? |
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. |
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. |