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

Luís Marques via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Oct 12 08:51:43 PDT 2020


luismarques added a comment.

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:

- Thanks for the thorough work for this patch and D89237 <https://reviews.llvm.org/D89237>. Much better than just tweaking the RISC-V stack spill limit, sweeping the issue under the rug and calling it a day!
- It might be a good idea to add additional reviewers, to check the tests for the other targets?
- 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.
- 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=...`.


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

https://reviews.llvm.org/D89239



More information about the llvm-commits mailing list