[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