Page MenuHomePhabricator

PowerPC: Add emergency stack spill slots for SPE
Needs ReviewPublic

Authored by jhibbits on Apr 22 2020, 2:01 PM.


Group Reviewers
Restricted Project

The powerpcspe 64-bit load/store only allows a 8-bit delta (32 64-bit
words), so if the stack size is any larger than that, we need extra
spill slots for doing indexing.

Diff Detail

Unit TestsFailed

9,080 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
1,520 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

jhibbits created this revision.Apr 22 2020, 2:01 PM
sfertile added inline comments.

Should probably extend this sentence to include (or 8-bit immediate for powerpcspe).


Is it possible to handwrite a simple test to expose the issue? If its too difficult we can stick with this test but I think something simpler which showcases the problem would be ideal.

jhibbits marked 3 inline comments as done.May 29 2020, 8:32 AM
jhibbits added inline comments.

Good point, will do.


I'll see if I can reduce it to something simple. This is actually the post-creduce result that exhibited the issue, any more reduction and it doesn't seem to trip it.

jhibbits marked 2 inline comments as done.Jun 6 2020, 8:33 PM
jhibbits added inline comments.

This is really odd, I am unable to reproduce the failure now, even rolling back to origin/release/10.x. The change needed is obviously correct, but being unable to test it is a problem.

Funny thing, I'm able to reproduce it *only* while running natively on a powerpcspe based device, not on any other device I've tested. I'll try to reduce the testcase even further, since it really is unwieldy.

jhibbits updated this revision to Diff 279526.Jul 21 2020, 8:10 AM

Simplify the test a little.

The problem with trying to write a simplified test by hand is it requires all
registers to be in use simultaneously, and all variables to be in scope
together, to exert register pressure. The best I can do is this test that's
slightly simplified (saves over 200 lines over the previous test) vs the
previous test. I hope it's sufficient.

jhibbits marked an inline comment as done.Jul 21 2020, 8:50 AM

As mentioned before, I can only seem to reproduce this running natively on powerpcspe; running the test on powerpc64, even without this patch, succeeds. This is extremely bizarre, but this fix gets the native build to produce the exact same text as running the test on powerpc64.


One thing to note: these flags are required to reproduce the problem, along with the !tbaa annotations in the code. Without them, even the otherwise failing llc would succeed.