[PATCH] D89239: [RISCV][PrologEpilogInserter] "Float" emergency spill slots to avoid making them immediately unreachable from the stack pointer

[Roger Ferrer Ibanez via Phabricator via llvm-commits]

rogfer01 added subscribers: sdesmalen, MatzeB, arsenm.
rogfer01 added a comment.

In D89239#2325224 <https://reviews.llvm.org/D89239#2325224>, @luismarques wrote:

> The code makes sense to me and it seems correct, but there may be assumptions about the way that mechanism operates that I'm not familiar with. Some comments:
>
> - It might be a good idea to add additional reviewers, to check the tests for the other targets?

Sure!

@sdesmalen @MatzeB mind to take a look at the AArch64 changes, they seem OK to me.

@arsenm is the AMDGPU change OK?

> - Nit: could the RISC-V test be further minimized? It still seems a bit big/complex. I'm assuming that's the minimized version of the test suite file that was failing to compile. Maybe also add a small comment in that file explaining what's being tested and describing the origin of the test.

I was able to reduce it a bit more with `llvm-reduce`. Not very noticeable but it was worth updating the test.

> - Could there be cases (due to future changes elsewhere) where these tests still pass but are no longer checking the conditions handled by the new code? If so, we could check that a debug run of the test prints one of the new debug strings (probably the one inside the loop). That's done in `llvm/test/CodeGen/RISCV/disjoint.ll`, by using `REQUIRES: asserts` and `-debug-only=...`.

So I've done two things here:

- to reduce the burden of maintaning the test I used `update_llc_test_checks.py` this time.
- I included your suggestion of using the debug message plus a `REQUIRES: asserts` so we can ensure we are going through the specific new path of PEI.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89239/new/

https://reviews.llvm.org/D89239