This is an archive of the discontinued LLVM Phabricator instance.

InstCombine: Introduce SimplifyDemandedUseFPClass
ClosedPublic

Authored by arsenm on Aug 23 2023, 11:16 AM.

Details

Summary

This is the floating-point analog of SimplifyDemandedBits. If we know
the edge cases are assumed impossible in uses, it's possible to prune
upstream edge case handling.

Start by only using this on returns in functions with nofpclass
returns (where I'm surprised there are no other combines), but this
can be extended to include any other nofpclass use or FPMathOperator
with flags.

Partially addresses issue #64870

Diff Detail

Event Timeline

arsenm created this revision.Aug 23 2023, 11:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2023, 11:16 AM
arsenm requested review of this revision.Aug 23 2023, 11:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2023, 11:16 AM
Herald added a subscriber: wdng. · View Herald Transcript
arsenm added inline comments.Aug 23 2023, 11:38 AM
llvm/test/Transforms/InstCombine/simplify-demanded-fpclass.ll
805–807

This is the real example and not sure how this ended up with a select i1 x, true

arsenm updated this revision to Diff 552873.Aug 23 2023, 1:47 PM

Add a few more real flavors of pow tests

nikic added a comment.Aug 25 2023, 3:46 AM

This looks conceptually fine to me.

llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
1825

Should this be using getFPClassConstant() instead, just like the main code path?

1846

It seems like this is correct, but only because fabs(FPClassTest) apparently does the exact opposite of what one would expect it to do. It's really inverse_fabs().

arsenm added inline comments.Aug 25 2023, 1:50 PM
llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
1825

No, otherwise it will infinite loop since constants will always yield the same result after simplification. The isa<UndefValue> check above is the special case that works for poison

nikic added inline comments.Aug 25 2023, 2:08 PM
llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
1825

Check that getFPClassConstant() != V?

arsenm updated this revision to Diff 554000.Aug 28 2023, 11:19 AM
goldstein.w.n added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
1825

No, otherwise it will infinite loop since constants will always yield the same result after simplification. The isa<UndefValue> check above is the special case that works for poison

1825

No, otherwise it will infinite loop since constants will always yield the same result after simplification. The isa<UndefValue> check above is the special case that works for poison

Isn't the same true in the many places you return I?

arsenm added inline comments.Sep 4 2023, 11:49 AM
llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
1825

No, because in those cases a change was made

goldstein.w.n added inline comments.Sep 4 2023, 1:26 PM
llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
1872

Can you not just return I->getOperand(1) here and I->getOperand(2) above?

arsenm updated this revision to Diff 556353.Sep 9 2023, 11:09 AM
arsenm marked an inline comment as done.
RKSimon added inline comments.Sep 13 2023, 5:49 AM
llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
1810

(style) assert message

1846

Do we need a llvm::inverse_fabs() ? Or at least a comment describing the behaviour

arsenm added inline comments.Sep 13 2023, 5:53 AM
llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
1846

This probably just needs to be renamed, this is what it is. Not sure that's the right name, make_sign_insensitive or something maybe? unknown_sign?

RKSimon added inline comments.Sep 13 2023, 7:11 AM
llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
1846

unknown_sign makes sense to me

arsenm added inline comments.Sep 13 2023, 7:16 AM
llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
1846

OK, I can do that in a separate change. this is a pre-existing function

arsenm updated this revision to Diff 556748.Sep 14 2023, 1:37 AM
arsenm marked an inline comment as done.

assert message, will separately rename FPClassTest helpers

LGTM - any other comments?

llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
1805

drop const?

nikic accepted this revision.Oct 4 2023, 6:20 AM

LGTM

This revision is now accepted and ready to land.Oct 4 2023, 6:20 AM
TWeaver added a subscriber: TWeaver.Oct 5 2023, 5:13 AM

Seeing as nobody has addressed the complaints from @antmo, I'm going to revert this patch to get the build bots green again.