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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
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
+
----------------
craig.topper wrote:
> 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.
> Is the empty basic block guaranteed to be at the end of the function in the ASM output.

For this input, it happens to be. I don't know about guaranteed though.

> The MIR shows the predecessor and successor lists which makes it more obvious.

So +2 then? :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109106