This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Enable shrink wrap by default
Needs RevisionPublic

Authored by evandro on Aug 31 2021, 5:59 PM.

Diff Detail

Event Timeline

evandro created this revision.Aug 31 2021, 5:59 PM
evandro requested review of this revision.Aug 31 2021, 5:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 31 2021, 5:59 PM

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.

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.

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.

asb accepted this revision.Sep 2 2021, 6:38 AM

This LGTM. It's a win across a bunch of the GCC torture suite tests (and no regressions).

This revision is now accepted and ready to land.Sep 2 2021, 6:38 AM
This revision was landed with ongoing or failed builds.Sep 2 2021, 7:49 AM
This revision was automatically updated to reflect the committed changes.
This revision is now accepted and ready to land.Feb 12 2022, 10:08 AM
jrtc27 requested changes to this revision.Feb 12 2022, 10:08 AM
This revision now requires changes to proceed.Feb 12 2022, 10:08 AM