[PATCH] D109106: [ISEL][BitTestBlock] pre-commit test for D109103

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/test/CodeGen/X86/SwitchLowering.ll:67
+; FIXME: Get rid of this conditional jump and bit test in .LBB1_1.
+; FIXME: .LBB1_4 should not have .LBB1_1 as a predacessor, or be past the end
+; FIXME: of the function.
----------------
predecessor*


================
Comment at: llvm/test/CodeGen/X86/switch-bit-test-unreachable-default.ll:3
+; RUN:   -stop-after=finalize-isel %s -o /dev/null 2>&1 | \
+; RUN:   FileCheck %s --check-prefix=CHECK-SISEL
+; RUN: llc -mtriple=x86_64-- -global-isel=1 -print-after=finalize-isel \
----------------
I think SDISEL would be a better abbreviation. It's used in other places in the codebase


================
Comment at: llvm/test/CodeGen/X86/switch-bit-test-unreachable-default.ll:6
+; RUN:   -stop-after=finalize-isel %s -o /dev/null 2>&1 | \
+; RUN:   FileCheck %s --check-prefix=CHECK-GISEL
+
----------------
nickdesaulniers wrote:
> hans wrote:
> > nickdesaulniers wrote:
> > > Is adding this much MIR withought llvm/utils/update_mir_test_checks.py being runable frowned upon?
> > > 
> > > llvm/utils/update_mir_test_checks.py doesn't work for this test, as written.
> > I don't know, but why are you doing MIR checks instead of the final assembly, which should be less verbose and therefore hopefully more robust?
> Fair question.
> 
> IMO ISEL is pass that makes the decisions here that are relevant, so ISEL is the unit that should be tested.
> 
> Running all backend passes is an integration test of multiple passes, and is what llvm/test/CodeGen/X86/SwitchLowering.ll does. So we have an existing integration test of all backend passes with: llvm/test/CodeGen/X86/SwitchLowering.ll, and newly added unit tests of ISEL with llvm/test/CodeGen/X86/switch-bit-test-unreachable-default.ll.
> 
> Would you prefer I convert llvm/test/CodeGen/X86/switch-bit-test-unreachable-default.ll to check ASM rather than MIR? Do other reviewers agree?
Is the empty basic block guaranteed to be at the end of the function in the ASM output. That's the only way to demonstrate the bug from the ASM I think. The MIR shows the predecessor and successor lists which makes it more obvious.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109106/new/

https://reviews.llvm.org/D109106