Page MenuHomePhabricator

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

Authored by jonpa on May 3 2017, 6:39 AM.

Details

Reviewers
atrick
uweigand
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)

Diff Detail

Event Timeline

jonpa created this revision.May 3 2017, 6:39 AM
atrick accepted this revision.May 9 2017, 12:00 AM

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.

This revision is now accepted and ready to land.May 9 2017, 12:00 AM
jonpa requested review of this revision.May 9 2017, 9:43 AM
jonpa edited edge metadata.

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 added a comment.May 9 2017, 9:52 AM

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 added a comment.May 9 2017, 9:57 AM

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 added a comment.May 9 2017, 9:59 AM

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...

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 added a comment.May 9 2017, 10:57 PM

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 updated this revision to Diff 98409.May 10 2017, 12:33 AM
jonpa retitled this revision from Add checks so that -pre-RA-sched=list-ilp does not crash on SystemZ (Bug 32723). to (Was: Add checks so that -pre-RA-sched=list-ilp does not crash on SystemZ (Bug 32723). ->) Implement getRepRegClassFor() in SystemZ..

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 accepted this revision.May 10 2017, 6:03 AM

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

This revision is now accepted and ready to land.May 10 2017, 6:03 AM
jonpa closed this revision.May 10 2017, 6:17 AM

Thanks. r302649