Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
The change makes sense, surprised this wasn't already the case. Have you done any kind of extensive testing to try and verify there aren't things we're missing that would break shrink wrapping (beyond the bug that motivated the original test case)? That'd really be my only concern.
llvm/test/CodeGen/RISCV/shrinkwrap.ll | ||
---|---|---|
2 | Is there value in keeping an -enable-shrink-wrap=false case? |
GCC's shrink wrapping is more sophisticated than LLVM's. LLVM's has very little tuning and doesn't support wrapping in more than one basic blocks. With this, future optimization will ... need to adjust the RISC-V tests but that's probably fine.
Yes. I kept this patch downstream for a couple of weeks or so and several benchmarks and test suites were run with it and not issues came up, even after multiple patches were applied on top of it.
llvm/test/CodeGen/RISCV/shrinkwrap.ll | ||
---|---|---|
2 | I don't believe so. But I don't feel strongly about it. |
This LGTM. It's a win across a bunch of the GCC torture suite tests (and no regressions).
Is there value in keeping an -enable-shrink-wrap=false case?