This is an archive of the discontinued LLVM Phabricator instance.

[SDAG] try to convert bit set/clear to signbit test when trunc is free
ClosedPublic

Authored by spatel on Dec 5 2022, 1:09 PM.

Details

Summary

This was noted as a regression in the post-commit feedback for D112634 (where we canonicalized IR differently).

For x86, this saves a few instruction bytes. AArch64 seems neutral.

Diff Detail

Event Timeline

spatel created this revision.Dec 5 2022, 1:09 PM
spatel requested review of this revision.Dec 5 2022, 1:09 PM

Looks reasonable to me.

What about if we limited this to just feeding select / brcond ?

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
3747

Move hasOneUse() to end of the if() condition as its heaviest test?

MatzeB accepted this revision.Dec 5 2022, 2:05 PM

Oh I also had started working on the same thing (should have been more clear that about that when talking in the other diff).

Anyway you went for the same solution (and managed to do in a couple fewer lines than me), thanks for working on it!

Added a comment about interesting effects in some tests, either way LGTM.

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
3749–3750

May want to check if NarrowVT is actually narrowed than the original type? (see test below).

llvm/test/CodeGen/X86/is_fpclass-fp80.ll
65–66 ↗(On Diff #480206)

I wonder if this is better or worse than before...

This revision is now accepted and ready to land.Dec 5 2022, 2:05 PM
spatel marked 3 inline comments as done.Dec 6 2022, 7:05 AM
spatel added inline comments.
llvm/test/CodeGen/X86/is_fpclass-fp80.ll
65–66 ↗(On Diff #480206)

I looked closer at the debug spew on this test. It is not dependent on the fact that the top bit is masked (and so the trunc disappears). Before legalization, we actually are shrinking from an i80 to i64.

So we can make this patch more focused by predicating on a legal source type, and that avoids the regression (I count this as a small regression because it needs an extra instruction even if it's likely cheap).

The pattern where the top bit is masked seems to be transformed independently of this patch for x86 at least, so we might want a follow-up patch to deal with that pattern specifically.

spatel updated this revision to Diff 480478.Dec 6 2022, 7:09 AM
spatel marked an inline comment as done.
spatel edited the summary of this revision. (Show Details)

Patch updated:

  1. Added source type legality check to avoid possible regression on x86 test; this also inhibits what looked like a small win for RISCV.
  2. Added TODO comment about easing that constraint.
  3. Moved hasOneUse() check to end of predicates.
spatel added a comment.Dec 6 2022, 7:18 AM

What about if we limited this to just feeding select / brcond ?

There's a maze of potential setcc and shift/logic folds, but I think this is the right match (more limited for now at least by the legality checks).

It should be better (less likely to miss folds) to adjust the larger pattern matches that reduce and/or of setcc via shifts/masks to recognize this form if needed.

RKSimon accepted this revision.Dec 6 2022, 7:20 AM

LGTM - cheers

This revision was landed with ongoing or failed builds.Dec 6 2022, 8:36 AM
This revision was automatically updated to reflect the committed changes.