Page MenuHomePhabricator

[AMDGPU] Remove -amdgpu-verify-regbanks-reassign
Needs ReviewPublic

Authored by foad on Jun 26 2020, 4:28 AM.

Details

Reviewers
rampitec
arsenm
Summary

There is a stall if two different sgpr operands come from the same bank,
unless they form an even-odd pair, in which case they are fetched
together and there is no stall even though they are in the same bank.

analyzeInst tries to count the number of stalls that an instruction
would have if all uses of Reg were replaced with a new register chosen
from Bank, but it can't do this accurately without knowing whether the
new register might form an even-odd pair with any of the other operands.

Because of this the CyclesSaved metric is not always accurate, so there
is no point in trying to verify it.

Diff Detail

Unit TestsFailed

TimeTest
10,230 mslinux > libomp.env::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/env/kmp_set_dispatch_buf.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/env/Output/kmp_set_dispatch_buf.c.tmp -lm -latomic && env KMP_DISP_NUM_BUFFERS=0 /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/env/Output/kmp_set_dispatch_buf.c.tmp
1,310 mslinux > libomp.worksharing/for::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/worksharing/for/kmp_set_dispatch_buf.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/worksharing/for/Output/kmp_set_dispatch_buf.c.tmp -lm -latomic && /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/worksharing/for/Output/kmp_set_dispatch_buf.c.tmp 7

Event Timeline

foad created this revision.Jun 26 2020, 4:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2020, 4:28 AM
foad added a comment.EditedJun 26 2020, 4:32 AM

Alternative solution #1: I think analyzeInst always returns the exact number of stalls or an overestimate, never an underestimate, so I could keep verifyCycles and change it to check the inequality StallCycles + CyclesSaved <= OriginalCycles.

Alternative solution #2: I could change tryReassign to scavenge a register from each bank before calling computeStallCycles, so we know exactly which register we're going to replace Reg with. But that might be expensive.

I like the alternative solution #1.

critson added a subscriber: critson.Aug 5 2020, 3:21 AM

I like the alternative solution #1.

I also agree with alt. solution #1.

To me a key aspect of the verifier is ensuring that forward progress is taking place.
For the bugs I was recently encountering this was true because CyclesSaved was going up, but StallCycles was not going down equivalently.
Because this was an infinite loop, CyclesSaved would keep growing indefinitely and the verifier would catch this.