This is an archive of the discontinued LLVM Phabricator instance.

Switch lowering: omit range check for bit tests when default is unreachable (PR43129)
ClosedPublic

Authored by hans on Sep 27 2019, 5:39 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

hans created this revision.Sep 27 2019, 5:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2019, 5:39 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
spatel added inline comments.Sep 27 2019, 7:46 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
2622 ↗(On Diff #222146)

Please pre-commit this and other cosmetic diffs.

llvm/test/CodeGen/X86/switch-bt.ll
161–179 ↗(On Diff #222146)

Please add this test with baseline CHECK lines as a pre-commit for this patch, so we show the functional difference from this patch.

Does this test cover both sides of the 'if' in the new code?

(MBB != NextBlock(SwitchBB))

and

(MBB == NextBlock(SwitchBB))

If not, can you add another test to provide that coverage?

Thanks for the patch! Just some small thoughts on style.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
2663 ↗(On Diff #222146)

Please prefer:

if true:
  foo()
else:
  bar()

to

if !true:
  bar()
else
  foo()
2670 ↗(On Diff #222146)

If you declared a SDValue before the conditional, you could simply initialize it in these branches, and share the setRoot assignment after the conditional.

hans marked 6 inline comments as done.Sep 30 2019, 1:50 AM
hans added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
2622 ↗(On Diff #222146)

r373191.

llvm/test/CodeGen/X86/switch-bt.ll
161–179 ↗(On Diff #222146)

Pre-committed a test in r373190.

I think constructing a test where MBB != NextBlock(SwitchBB) would be very tricky.

hans updated this revision to Diff 222373.Sep 30 2019, 1:51 AM
hans marked 2 inline comments as done.

Rebasing and addressing comments.

Please take another look.

spatel added inline comments.Sep 30 2019, 6:50 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
2630–2633 ↗(On Diff #222373)

Can we sink RangeCmp into the !OmitRangeCheck block below? As written, we are potentially creating a node with no uses?

2666–2667 ↗(On Diff #222373)

Is the optional branch to MBB identical on both sides of this if/else? If so, sink it after the if/else (and that simplifies the if/else to just !OmitRangeCheck)?

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
2674 ↗(On Diff #222373)

This statement and conditional matches L2667. I think we could flip the conditionals/check for this first? DRY

hans marked 4 inline comments as done.Oct 1 2019, 1:26 AM
hans added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
2630–2633 ↗(On Diff #222373)

It's a bit annoying, because by the time we're down here, the code above has changed the value of Sub which we would use in RangeCmp. But that can be fixed of course.. doing that.

hans updated this revision to Diff 222558.Oct 1 2019, 1:27 AM
hans marked an inline comment as done.

Please take another look.

xbolva00 added inline comments.
llvm/test/CodeGen/X86/switch-bt.ll
165 ↗(On Diff #222558)

Can you (re)generate CHECKs with update_llc_test_checks.py?

hans marked an inline comment as done.Oct 1 2019, 2:20 AM
hans added inline comments.
llvm/test/CodeGen/X86/switch-bt.ll
165 ↗(On Diff #222558)

Why? It's a hand-written test, and the point of the patch is specifically to omit the cmp here.

nickdesaulniers added inline comments.Oct 1 2019, 9:52 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
2650 ↗(On Diff #222558)

So it seems to me that CopyTo might not even end up as the root. It looks like we have 3 cases that affect the root:

  1. !B.OmitRangeCheck
  2. MBB != NextBlock(SwitchBB)
  3. Neither 1 or 2

For cases 1 and 2, we do all this wasteful work to create CopyTo and Sub, just to throw it away. It seems like RangeSub isn't needed for case 2. I think it would be clearer and do less work for each case to have a simple 3 arm conditional, rather than successive conditionals that may just throw away previous work to reassign to Root.

hans marked 2 inline comments as done.Oct 1 2019, 12:29 PM
hans added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
2650 ↗(On Diff #222558)

Note that Root below isn't just assigned to, it's also passed as an argument when creating BRCOND or the BR node.

So in the cases where CopyTo doesn't end up being the root, it's been passed as the chain element to one of those instructions. In all cases, it is on the chain -- it's never thrown away. (And neither is RangeSub since it's an input to CopyTo.)

I realize that's maybe not super clear, but I think this style of updating the selection dag root by using the old root as the chain of the new root is common.

nickdesaulniers marked an inline comment as done.Oct 1 2019, 1:25 PM
nickdesaulniers added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
2650 ↗(On Diff #222558)

yep, sorry I missed that.

spatel accepted this revision.Oct 1 2019, 2:38 PM

LGTM

This revision is now accepted and ready to land.Oct 1 2019, 2:38 PM
This revision was automatically updated to reflect the committed changes.
hans marked an inline comment as done.
RKSimon added a subscriber: RKSimon.Oct 2 2019, 4:17 AM

@hans This is causing EXPENSIVE_CHECKS failures, please can you take a look? http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/19967

hans added a comment.Oct 2 2019, 5:06 AM

@hans This is causing EXPENSIVE_CHECKS failures, please can you take a look? http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/19967

Sorry about that. Reverted in r373454 until I can investigate it properly. Hopefully it's just a simple fix :-)

hans added a comment.Oct 2 2019, 7:33 AM

Re-landed with fix in r373477.