This is an archive of the discontinued LLVM Phabricator instance.

InstCombine: Match pattern that appears in clang's __builtin_isnormal
ClosedPublic

Authored by arsenm on Dec 5 2022, 5:15 AM.

Details

Summary

and (fcmp ord x, 0), (fcmp u* x, inf) -> fcmp o* x, inf
and (fcmp ord x, 0), (fcmp u* fabs(x), inf) -> fcmp o* x, inf

Clang emits this peculiar pattern as an isfinite check in
__builtin_isnormal which can be simplified. We should fix clang to
emit this in the first place, but should also fold it here.

Diff Detail

Event Timeline

arsenm created this revision.Dec 5 2022, 5:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 5 2022, 5:15 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
arsenm requested review of this revision.Dec 5 2022, 5:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 5 2022, 5:15 AM
Herald added a subscriber: wdng. · View Herald Transcript
foad added a comment.Dec 5 2022, 5:37 AM

Looks reasonable to me. It could be relaxed a bit (e.g. the constants 0 and inf could be any non-nan constant values) but I guess there's no particular use case for that.

arsenm added a comment.Dec 5 2022, 5:56 AM

Looks reasonable to me. It could be relaxed a bit (e.g. the constants 0 and inf could be any non-nan constant values) but I guess there's no particular use case for that.

I thought about handling non-canonical fcmp ord/uno, but I can't write a test that meaningfully changes for that

foad added inline comments.Dec 12 2022, 2:09 PM
llvm/lib/IR/Instructions.cpp
4098 ↗(On Diff #480061)

This whole function is just return Pred & 7;. See the definition of CmpInst::Predicate. The floating point predicates are carefully numbered to make exactly this kind of thing possible.

arsenm updated this revision to Diff 482826.Dec 14 2022, 5:46 AM

Use & FCMP_ORD

foad added inline comments.Dec 14 2022, 6:29 AM
llvm/include/llvm/IR/InstrTypes.h
839

Argument name should be capitalized (though the existing functions are inconsistent on this).

llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
1229

Comment seems wrong - this function doesn't do anything with fabs.

1307

Do you need to worry about IsLogicalSelect here?

arsenm added inline comments.Dec 14 2022, 7:05 AM
llvm/include/llvm/IR/InstrTypes.h
839

Right this was just copy paste from the others

llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
1229

It's not wrong, it's just the fabs stripping is applied in the predicate where it's called

1307

I don't really know what IsLogicalSelect means

arsenm added inline comments.Dec 14 2022, 8:35 AM
llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
1307

I reproduced a case from a select and alive2 seems to not have an issue with it

arsenm updated this revision to Diff 482884.Dec 14 2022, 9:04 AM

Add test coverage for logical select case. Alive says it's fine

foad added inline comments.Dec 14 2022, 11:05 PM
llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
1229

I think the comment on this function should say what this function does. I can't see how this function strips anything. Also what is y in the comment?

sepavloff added inline comments.Dec 16 2022, 12:52 AM
llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
1217–1220

As this is fcmp ord, the name matchIsNotNaN would be better.

IR producer (not necessarily clang) may chose any form of NaN check, so the check must test both forms, fcmp ord x, const or fcmp ord x, x.

1232

The function name is not clear enough. Why not matchFiniteTest() or something similar?

arsenm added inline comments.Dec 16 2022, 4:22 AM
llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
1217–1220

And that get canonicalized into fcmp ord with a zero already. Clang actually emits a non canonical ordered check in isnormal as it is

arsenm updated this revision to Diff 483536.Dec 16 2022, 7:27 AM
arsenm marked 2 inline comments as done.

Rename functions, move comments.

Also stop bothering with getFCmpValue because we can't end up with a compare true/false here

foad accepted this revision.Dec 19 2022, 1:58 AM
This revision is now accepted and ready to land.Dec 19 2022, 1:58 AM