Add missing fabs(fpext) optimzation that worked with the call,
and also fixes it creating a second fpext when there were multiple
uses.
Details
- Reviewers
efriedma
Diff Detail
Event Timeline
Canonicalizing @fabs() -> @llvm.fabs() makes sense.
I'd like to see a regression test which specifically checks the fabs/fabsf/fabsl -> llvm.fabs transform. Please include testcases which transform fabsl -> llvm.fabs.f64, fabsl -> llvm.fabs.f80, and fabsl -> llvm.fabs.f128, to make sure they all work as expected.
Please make sure to kill off all the existing optimizations besides this one which check for LibFunc::fabs or the string "fabsf" in a followup.
test/Transforms/InstCombine/double-float-shrink-2.ll | ||
---|---|---|
27 | "replacable" is a little unclear... I think the point is that the intrinsic does the right thing on all platforms? | |
test/Transforms/InstCombine/fabs.ll | ||
19–20 | This is probably going to fail on Windows... maybe change the test to call llvm.fabs rather than fabs? |
lib/Transforms/InstCombine/InstCombineCalls.cpp | ||
---|---|---|
1642 | Do we need to preserve fast-math flags here? | |
lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
1221 | Do we need to preserve fast-math flags here? | |
test/Transforms/InstCombine/double-float-shrink-2.ll | ||
91 | We allow putting fast-math flags on arbitrary calls...? Please update LangRef with an explanation of what this means. |
lib/Transforms/InstCombine/InstCombineCalls.cpp | ||
---|---|---|
1642 | I don't think fpext supports fast math flags |
test/Transforms/InstCombine/double-float-shrink-2.ll | ||
---|---|---|
91 | FMF on calls was added in r255555 |
test/Transforms/InstCombine/double-float-shrink-2.ll | ||
---|---|---|
91 | This seems to already be noted in the langref for call |
test/Transforms/InstCombine/fabs.ll | ||
---|---|---|
19–20 | Do you mean the intrinsic won't work? I would expect codegen to promote it |
Test fp80
test/Transforms/InstCombine/fabs.ll | ||
---|---|---|
19–20 | It looks like a fabs instruction is emitted for the intrinsic |
test/Transforms/InstCombine/fabs.ll | ||
---|---|---|
19–20 | I thought the intent there was to not recognize fabsf as a special builtin function since it isn't available, i.e. fabsf could be a user function |
test/Transforms/InstCombine/fabs.ll | ||
---|---|---|
19–20 | Yes, that's working as intended; the only problem is that you didn't specify a triple for this test, so it will inherit the triple from the host, therefore on a Windows host the CHECK line won't match. |
Fast-math flags?