This is an archive of the discontinued LLVM Phabricator instance.

[Instcombine] Add patterns to generate fneg(fabs(x)) instead of fcmp/sub/selects (part 1)
Changes PlannedPublic

Authored by mnadeem on Oct 27 2021, 4:15 PM.

Details

Reviewers
spatel
efriedma
Summary

https://alive2.llvm.org/ce/z/L5DmiE

TODO: handle <=, >, and >=

Diff Detail

Event Timeline

mnadeem created this revision.Oct 27 2021, 4:15 PM
mnadeem requested review of this revision.Oct 27 2021, 4:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2021, 4:15 PM
mnadeem added inline comments.Oct 27 2021, 4:21 PM
llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
2970

<= needs nsz on the select, so I'll just add that condition here and the rest should work fine.

arsenm added a subscriber: arsenm.Oct 27 2021, 4:34 PM
arsenm added inline comments.
llvm/test/Transforms/InstCombine/fneg-fabs.ll
28

Missing some cases where the compared value is -0?

mnadeem added inline comments.Oct 27 2021, 5:03 PM
llvm/test/Transforms/InstCombine/fneg-fabs.ll
28

Any specific cases? Or should I duplicate all tests and set the compared value as -0?

Thanks for splitting this up and creating the Alive link!

I'm still not clear if the FMF propagation is behaving as well as it could. For example, if the select has nsz, then can it always be added to the fneg?
https://alive2.llvm.org/ce/z/2Qq6Pz

It would also help to sprinkle some extra flags like nnan and ninf into the tests, so we can tell how those are modified even though they are not required for the transform.

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
2968

"--> -X" should be "--> -fabs(X)"

llvm/test/Transforms/InstCombine/fneg-fabs.ll
28

We canonicalize fcmp to +0.0, so I think it would be enough to just switch any one of these tests to confirm that.
Using different types (a vector, a float, a half, etc) as we cycle through these would also increase coverage.

mnadeem added a subscriber: efriedma.

I'm still not clear if the FMF propagation is behaving as well as it could. For example, if the select has nsz, then can it always be added to the fneg?
https://alive2.llvm.org/ce/z/2Qq6Pz

@efriedma any opinion on this?

If the select is nsz, that means that if the result is zero, we don't care about its sign. That means we could copy it onto the fneg, sure. Actually, the produced fabs and fneg should always be nsz; the transform doesn't make sense otherwise.

For other flags, you can probably just copy them from the select? So initialize the flags from the select, then unconditionally enable the nsz bit for both instructions, I guess.

mnadeem planned changes to this revision.Feb 27 2023, 11:54 AM

Removing from reviewer's ready to review list for now. Will come back to this patch when/if time permits.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 27 2023, 11:54 AM
Herald added a subscriber: StephenFan. · View Herald Transcript