[SystemZ] Bugfix in emitSelect()

[jonpa]

Summary

When more than one SelectPseudo instruction is handled a new MBB is returned. This must not be done if that would result in leaving an undhandled isel pseudo behind in the original MBB.

Fixes https://bugs.llvm.org/show_bug.cgi?id=44849.

[jonpa]

BTW, this was an example where the backend messed up and left behind a pseudo MI (that uses custom insertion) after the FinalizeISel pass. This was a bit suprising to me as in my memory there was an assert for this that checked that all pseudos were gone after the loop over MF. Perhaps that would still be good to have..?

[uweigand]

llvm/lib/Target/SystemZ/SystemZISelLowering.cpp

6889

Isn't this always true for select pseudos? Why doesn't make the rest of this loop a no-op?

[jonpa]

Do the check after checking for a select pseudo. Moved down also the check for CC-def, since a SelectPseudo does not define CC.

llvm/lib/Target/SystemZ/SystemZISelLowering.cpp

6889

Oops! Sorry.

[uweigand]

I guess it would be nicer if we could still handle this case somehow.

But anyway, for now bailing out is certainly preferable to aborting compilation.

LGTM.

llvm/lib/Target/SystemZ/SystemZISelLowering.cpp

6889

Interesting that this didn't cause any test to fail ... Don't we have tests that verify this loop does something?

[jonpa]

llvm/lib/Target/SystemZ/SystemZISelLowering.cpp

6889

We do and they were failing, it was just me who was too happy finding the cause of this problem and then forgetting to run the tests before submitting...

[fhahn]

It seems like the test added in this patch fails with expensive checks enabled: http://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-expensive/15376/testReport/junit/LLVM/CodeGen_SystemZ/multiselect_02_mir/

It would be great if you could take a look.

[jonpa]

In D74352#1921430, @fhahn wrote:

It seems like the test added in this patch fails with expensive checks enabled: http://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-expensive/15376/testReport/junit/LLVM/CodeGen_SystemZ/multiselect_02_mir/

It would be great if you could take a look.

This test has been running fine for a while with expensive checks enables, so I don't quite understand why this is failing on that particular build...

I tried to use the same cmake options as the Jenkins job did:

cmake -G Ninja -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra;compiler-rt;libcxx" -DLLVM_ENABLE_LTO=Off -DLLVM_BUILD_EXAMPLES=On -DCMAKE_MACOSX_RPATH=On -DCMAKE_BUILD_TYPE=Release -DLLVM_BUILD_EXTERNAL_COMPILER_RT=On -DLLVM_ENABLE_EXPENSIVE_CHECKS=ON -DLIBCXX_ENABLE_SHARED=OFF -DLIBCXX_ENABLE_STATIC=OFF -DLIBCXX_INCLUDE_TESTS=OFF -DLLVM_ENABLE_ASSERTIONS=On $LLVM_SRC/llvm

All tests are passing, on 7b74b0d (March 13).

I see in the Jenkins log "Checking out Revision 7eeee2a196c01bd43fa468339d39251faac1a88e (refs/remotes/origin/master)", but that is not a commit in my repo...

[fhahn]

In D74352#1921939, @jonpa wrote:

In D74352#1921430, @fhahn wrote:

It seems like the test added in this patch fails with expensive checks enabled: http://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-expensive/15376/testReport/junit/LLVM/CodeGen_SystemZ/multiselect_02_mir/

It would be great if you could take a look.

This test has been running fine for a while with expensive checks enables, so I don't quite understand why this is failing on that particular build...

I tried to use the same cmake options as the Jenkins job did:

cmake -G Ninja -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra;compiler-rt;libcxx" -DLLVM_ENABLE_LTO=Off -DLLVM_BUILD_EXAMPLES=On -DCMAKE_MACOSX_RPATH=On -DCMAKE_BUILD_TYPE=Release -DLLVM_BUILD_EXTERNAL_COMPILER_RT=On -DLLVM_ENABLE_EXPENSIVE_CHECKS=ON -DLIBCXX_ENABLE_SHARED=OFF -DLIBCXX_ENABLE_STATIC=OFF -DLIBCXX_INCLUDE_TESTS=OFF -DLLVM_ENABLE_ASSERTIONS=On $LLVM_SRC/llvm

All tests are passing, on 7b74b0d (March 13).

I see in the Jenkins log "Checking out Revision 7eeee2a196c01bd43fa468339d39251faac1a88e (refs/remotes/origin/master)", but that is not a commit in my repo...

Hm, it looks like it also failed on Marc 5th, and there have been no builds in between. (http://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-expensive/15374/). Let's keep an eye out and see if it keeps failing.

llvm/lib/Target/SystemZ/SystemZISelLowering.cpp

SystemZTargetLowering::emitSelect(MachineInstr &MI,
   // a single pair of basic blocks using the same condition.
   SmallVector<MachineInstr*, 8> Selects;
   SmallVector<MachineInstr*, 8> DbgValues;
   Selects.push_back(&MI);
   unsigned Count = 0;
   for (MachineBasicBlock::iterator NextMIIt =
          std::next(MachineBasicBlock::iterator(MI));
        NextMIIt != MBB->end(); ++NextMIIt) {
-    if (NextMIIt->definesRegister(SystemZ::CC))
-      break;
     if (isSelectPseudo(*NextMIIt)) {
       assert(NextMIIt->getOperand(3).getImm() == CCValid &&
              "Bad CCValid operands since CC was not redefined.");
       if (NextMIIt->getOperand(4).getImm() == CCMask ||
           NextMIIt->getOperand(4).getImm() == (CCValid ^ CCMask)) {
         Selects.push_back(&*NextMIIt);
         continue;
       }
       break;
     }
+    if (NextMIIt->definesRegister(SystemZ::CC) ||
+        NextMIIt->usesCustomInsertionHook())
+      break;
     bool User = false;
     for (auto SelMI : Selects)
       if (NextMIIt->readsVirtualRegister(SelMI->getOperand(0).getReg())) {
         User = true;
         break;
       }
     if (NextMIIt->isDebugInstr()) {
       if (User) {

llvm/test/CodeGen/SystemZ/multiselect-02.mir

+# RUN: llc -mtriple=s390x-linux-gnu -mcpu=z10 -run-pass=finalize-isel -o - %s \
+# RUN:   | FileCheck %s
+#
+# Test that an instruction (ZEXT128) that uses custom insertion gets treated
+# correctly also when it lies between two Select instructions that could
+# potentially be handled together.
+#
+# CHECK-LABEL: bb.0.entry:
+# CHECK-NOT: ZEXT128
+
+--- |
+  declare void @bar(i32)
+  define i32 @fun() { entry: ret i32 0 }
+---
+name:            fun
+body:             |
+  bb.0.entry:
+    %1:addr64bit = IMPLICIT_DEF
+    %0:gr32bit = LLC %1, 0, $noreg :: (load 1 from `i8* undef`)
+    CHI killed %0, 0, implicit-def $cc
+    %2:gr32bit = LHI 2
+    %3:gr32bit = LHI 8
+    %4:gr32bit = Select32 killed %3, killed %2, 14, 8, implicit $cc
+    %5:gr32bit = LHI 128
+    %7:gr64bit = IMPLICIT_DEF
+    %6:gr64bit = INSERT_SUBREG %7, killed %5, %subreg.subreg_l32
+    %8:gr128bit = ZEXT128 killed %6
+    %10:addr64bit = IMPLICIT_DEF
+    %9:gr128bit = DL %8, %10, 0, $noreg :: (load 4 from `i64* undef` + 4)
+    %11:gr32bit = COPY %9.subreg_l32
+    %12:gr64bit = LGHI 2
+    %13:gr64bit = LGHI 8
+    %14:gr64bit = Select64 killed %13, killed %12, 14, 8, implicit $cc
+    CR %4, %11, implicit-def $cc
+    %15:gr32bit = Select32 %11, %4, 14, 4, implicit $cc
+    ADJCALLSTACKDOWN 0, 0
+    $r2d = COPY %14
+    CallBRASL @bar, $r2d, csr_systemz, implicit-def dead $r14d, implicit-def dead $cc, implicit $fpc
+    ADJCALLSTACKUP 0, 0
+    $r2l = COPY %15
+    Return implicit $r2l
+
+...