(Was: Add checks so that -pre-RA-sched=list-ilp does not crash on SystemZ (Bug 32723). ->) Implement getRepRegClassFor() in SystemZ.

[jonpa]

Summary

Due to the current use in the SystemZ backend of MVT::Untyped, the list-ilp scheduler will crash when it does look ups like

TLI->getRepRegClassFor(VT)->getID();

This patch adds checks for MVT::Untyped in the places needed to avoid compiler crashes resulting when VT is Untyped.

Discussion on https://bugs.llvm.org//show_bug.cgi?id=32723. (This was posted on llvm-commits last week also without reply)

[jonpa]

ping!

[atrick]

I'm haven't looked at this code in a long time, and I have no idea if it makes sense to have an Untyped MVT at this point in the code. But... your fix looks harmless.

[jonpa]

Hi Andy,

thanks for taking a look!

I ran the regression tests before committing, and found to my surprise that two tests had now changed:

test/CodeGen/Mips/divrem.ll
test/CodeGen/Mips/llvm-ir/mul.ll

I thought this was just SystemZ that was lagging a bit, but it seems that Mips also has Untyped values, while also having a untyped register class (ACC64DSP). This means that the register pressure heuristics actually was in use for Untyped...

Now I am not sure anymore what to do. Is it reasonable to update the tests on Mips with the new output? Or should register pressure be correct also for Untyped? What I saw so far was minor changes not causing any spilling or so, just some reordering, like:

TRUNK:                                                                          W/ PATCH:
sdivrem1:                               # @sdivrem1                             sdivrem1:                               # @sdivrem1
        .frame  $sp,0,$ra                                                               .frame  $sp,0,$ra
        .mask   0x00000000,0                                                            .mask   0x00000000,0
        .fmask  0x00000000,0                                                            .fmask  0x00000000,0
        .set    noreorder                                                               .set    noreorder
        .set    nomacro                                                                 .set    nomacro
        .set    noat                                                                    .set    noat
# BB#0:                                 # %entry                                # BB#0:                                 # %entry
        div     $zero, $4, $5                                                           div     $zero, $4, $5
        teq     $5, $zero, 7                                                            teq     $5, $zero, 7
        mflo    $2                                                        <
        mfhi    $1                                                                      mfhi    $1
        jr      $ra                                                       <
        sw      $1, 0($6)                                                               sw      $1, 0($6)
                                                                          >             jr      $ra
                                                                          >             mflo    $2
        .set    at                                                                      .set    at
        .set    macro                                                                   .set    macro
        .set    reorder                                                                 .set    reorder
        .end    sdivrem1                                                                .end    sdivrem1

udivrem1:                               # @udivrem1                             udivrem1:                               # @udivrem1
        .frame  $sp,0,$ra                                                               .frame  $sp,0,$ra
        .mask   0x00000000,0                                                            .mask   0x00000000,0
        .fmask  0x00000000,0                                                            .fmask  0x00000000,0
        .set    noreorder                                                               .set    noreorder
        .set    nomacro                                                                 .set    nomacro
        .set    noat                                                                    .set    noat
# BB#0:                                 # %entry                                # BB#0:                                 # %entry
        divu    $zero, $4, $5                                                           divu    $zero, $4, $5
        teq     $5, $zero, 7                                                            teq     $5, $zero, 7
        mflo    $2                                                        <
        mfhi    $1                                                                      mfhi    $1
        jr      $ra                                                       <
        sw      $1, 0($6)                                                               sw      $1, 0($6)
                                                                          >             jr      $ra
                                                                          >             mflo    $2
        .set    at                                                                      .set    at
        .set    macro                                                                   .set    macro
        .set    reorder                                                                 .set    reorder
        .end    udivrem1                                                                .end    udivrem1

I of course tried to use

./utils/update_llc_test_checks.py ./test/CodeGen/Mips/llvm-ir/mul.ll

but got

Cannot find a triple. Assume 'x86'
Cannot find a triple. Assume 'x86'
WARNING: Found conflicting asm under the same prefix: 'GP32'!
WARNING: Found conflicting asm under the same prefix: 'GP32'!
WARNING: Found conflicting asm under the same prefix: 'GP32'!
WARNING: Found conflicting asm under the same prefix: 'GP32'!
WARNING: Found conflicting asm under the same prefix: 'GP32'!
Cannot find a triple. Assume 'x86'
WARNING: Found conflicting asm under the same prefix: 'GP32'!
WARNING: Found conflicting asm under the same prefix: 'GP32'!
Cannot find a triple. Assume 'x86'
Cannot find a triple. Assume 'x86'
Cannot find a triple. Assume 'x86'
WARNING: Found conflicting asm under the same prefix: 'GP32'!
WARNING: Found conflicting asm under the same prefix: 'GP32'!
Cannot find a triple. Assume 'x86'
Cannot find a triple. Assume 'x86'
WARNING: Found conflicting asm under the same prefix: 'GP64-NOT-R6'!
WARNING: Found conflicting asm under the same prefix: 'GP64-NOT-R6'!
WARNING: Found conflicting asm under the same prefix: 'GP64-NOT-R6'!
WARNING: Found conflicting asm under the same prefix: 'GP64-NOT-R6'!
Cannot find a triple. Assume 'x86'
WARNING: Found conflicting asm under the same prefix: 'GP64-NOT-R6'!
WARNING: Found conflicting asm under the same prefix: 'GP64-NOT-R6'!
Cannot find a triple. Assume 'x86'
Cannot find a triple. Assume 'x86'
Cannot find a triple. Assume 'x86'
Cannot find a triple. Assume 'x86'
Cannot find a triple. Assume 'x86'
Cannot find a triple. Assume 'x86'

The script was obviously not working, because the updated test also failed.

Should I aim to update the tests, or should I instead go though the work of adding an untyped regclass the same way Mips does it? (This would be a temporary solution, as in fact the best solution would be to avoid the Untyped and make i128 a legal type).

[MatzeB]

Just out of interested: What's the interested in these selection dag schedulers, I though they are deprecated and only SourceListDAGScheduler and possibly SchedulerDAGFast are of interest with the MachineScheduler around.

Is this because of the same reasons (or same benchmark?) that motivated https://reviews.llvm.org/D32563 ?

[jonpa]

In D32802#749851, @MatzeB wrote:

Just out of interested: What's the interested in these selection dag schedulers, I though they are deprecated and only SourceListDAGScheduler and possibly SchedulerDAGFast are of interest with the MachineScheduler around.

Is this because of the same reasons (or same benchmark?) that motivated https://reviews.llvm.org/D32563 ?

This was just a bug I discovered with llvm-stress. As long as this option is around, it shouldn't mean that the compiler crashes when it is used, right? (Perhaps an exit with some kind of message explaining that this scheduler is unsupported might due instead...?)

[MatzeB]

In D32802#749855, @jonpa wrote:

In D32802#749851, @MatzeB wrote:

Just out of interested: What's the interested in these selection dag schedulers, I though they are deprecated and only SourceListDAGScheduler and possibly SchedulerDAGFast are of interest with the MachineScheduler around.

Is this because of the same reasons (or same benchmark?) that motivated https://reviews.llvm.org/D32563 ?

This was just a bug I discovered with llvm-stress. As long as this option is around, it shouldn't mean that the compiler crashes when it is used, right? (Perhaps an exit with some kind of message explaining that this scheduler is unsupported might due instead...?)

As far as I am concerned I would happily remove this option. But as usual I don't know why it wasn't removed when the MachineScheduler was introduced and who/what targets may be depending on it...

[MatzeB]

In D32802#749855, @jonpa wrote:

In D32802#749851, @MatzeB wrote:

Just out of interested: What's the interested in these selection dag schedulers, I though they are deprecated and only SourceListDAGScheduler and possibly SchedulerDAGFast are of interest with the MachineScheduler around.

Is this because of the same reasons (or same benchmark?) that motivated https://reviews.llvm.org/D32563 ?

This was just a bug I discovered with llvm-stress. As long as this option is around, it shouldn't mean that the compiler crashes when it is used, right? (Perhaps an exit with some kind of message explaining that this scheduler is unsupported might due instead...?)

llvm-stress is using commandline options to change the selection dag scheduler?

[jonpa]

In D32802#749860, @MatzeB wrote:

In D32802#749855, @jonpa wrote:

In D32802#749851, @MatzeB wrote:

Just out of interested: What's the interested in these selection dag schedulers, I though they are deprecated and only SourceListDAGScheduler and possibly SchedulerDAGFast are of interest with the MachineScheduler around.

Is this because of the same reasons (or same benchmark?) that motivated https://reviews.llvm.org/D32563 ?

This was just a bug I discovered with llvm-stress. As long as this option is around, it shouldn't mean that the compiler crashes when it is used, right? (Perhaps an exit with some kind of message explaining that this scheduler is unsupported might due instead...?)

llvm-stress is using commandline options to change the selection dag scheduler?

I was trying different llc options to find more bugs with llvm-stress. This way I would get a bigger set of different inputs to the various CodeGen passes. Changing isel scheduler is one way of changing things around.

[jonpa]

At second thought, since the tests failed, it might be easier to do like Mips and just implement getRepRegClassFor() for MVT::Untyped, just so that the schedulers do not crash.

As far as I am concerned I would happily remove this option. But as usual I don't know why it wasn't removed when the MachineScheduler was introduced and who/what targets may be depending on it...

Unfortunately, SystemZ is still using Sched::RegPressure, because the pre-ra mischeduler has so far not been used without regressions. This will hopefully be changed soon. It seems that other targets are still calling setSchedulingPreference() with the different schedulers, so this is then perhaps true also on other targets?

[uweigand]

This new back-end only change is fine with me, as long list-ilp is still there. LGTM.

[jonpa]

Thanks. r302649

lib/Target/SystemZ/SystemZISelLowering.h

private:
   MachineBasicBlock *emitStringWrapper(MachineInstr &MI, MachineBasicBlock *BB,
                                        unsigned Opcode) const;
   MachineBasicBlock *emitTransactionBegin(MachineInstr &MI,
                                           MachineBasicBlock *MBB,
                                           unsigned Opcode, bool NoFloat) const;
   MachineBasicBlock *emitLoadAndTestCmp0(MachineInstr &MI,
                                          MachineBasicBlock *MBB,
                                          unsigned Opcode) const;
+
+  const TargetRegisterClass *getRepRegClassFor(MVT VT) const override;
 };
 } // end namespace llvm
 
 #endif

lib/Target/SystemZ/SystemZISelLowering.cpp

case SystemZ::LTDBRCompare_VecPseudo:
     return emitLoadAndTestCmp0(MI, MBB, SystemZ::LTDBR);
   case SystemZ::LTXBRCompare_VecPseudo:
     return emitLoadAndTestCmp0(MI, MBB, SystemZ::LTXBR);
 
   default:
     llvm_unreachable("Unexpected instr type to insert");
   }
 }
+
+// This is only used by the isel schedulers, and is needed only to prevent
+// compiler from crashing when list-ilp is used.
+const TargetRegisterClass *
+SystemZTargetLowering::getRepRegClassFor(MVT VT) const {
+  if (VT == MVT::Untyped)
+    return &SystemZ::ADDR128BitRegClass;
+  return TargetLowering::getRepRegClassFor(VT);
+}

test/CodeGen/SystemZ/list-ilp-crash.ll

+; RUN: llc < %s -mtriple=s390x-linux-gnu -mcpu=z13 -pre-RA-sched=list-ilp | FileCheck %s
+;
+; Check that list-ilp scheduler does not crash due to SystemZ's current use
+; of MVT::Untyped.
+
+define void @pr32723(i8) {
+; CHECK: .text
+BB:
+  br label %CF245
+
+CF245:                                            ; preds = %CF245, %BB
+  %Shuff57 = shufflevector <4 x i8> zeroinitializer, <4 x i8> zeroinitializer, <4 x i32> <i32 0, i32 2, i32 4, i32 6>
+  %Cmp84 = icmp uge i8 %0, undef
+  br i1 %Cmp84, label %CF245, label %CF260
+
+CF260:                                            ; preds = %CF245
+  %B156 = sdiv <4 x i8> %Shuff57, %Shuff57
+  br label %CF255
+
+CF255:                                            ; preds = %CF255, %CF260
+  %I186 = insertelement <4 x i8> %B156, i8 %0, i32 2
+  br label %CF255
+}