This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Disable subregister liveness by default
ClosedPublic

Authored by frasercrmck on Jul 13 2022, 8:02 AM.

Details

Summary

We previously enabled subregister liveness by default when compiling
with RVV. This has been shown to cause miscompilations where RVV
register operand constraints are not met. A test was added for this in
D129639 which explains the issue in more detail.

Until this issue is fixed in some way, we should not be enabling
subregister liveness unless the user asks for it.

Diff Detail

Event Timeline

frasercrmck created this revision.Jul 13 2022, 8:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2022, 8:02 AM
frasercrmck requested review of this revision.Jul 13 2022, 8:02 AM
rogfer01 accepted this revision.Jul 13 2022, 12:50 PM

I'm too in favour of disabling subregister liveness until we can model the problematic instructions constraints in a more robust way.

This revision is now accepted and ready to land.Jul 13 2022, 12:50 PM
craig.topper accepted this revision.Jul 13 2022, 4:10 PM

I agree we should disable this.

Another approach to fix/work-around for this issue: https://reviews.llvm.org/D129735

Personally I would prefer keep sub register liveness enabled by default, that indeed improved code gen quality especially when program contain segment load/store.

As I can see the most root cause is the undef and poison value in the program, ideally that should not appeared in program IMO, so I would prefer to trade some code gen quality about undef/poison value to make sub register liveness still enabled, and buy more time to model the constraint issue right.

kito-cheng accepted this revision.Jul 14 2022, 3:36 AM

I guess I should express that explicitly: I am not intend to block this patch and OK to disable by default unless we have another better way, since apparently the alternative solution https://reviews.llvm.org/D129735 isn't ready.

This revision was landed with ongoing or failed builds.Jul 14 2022, 9:15 AM
This revision was automatically updated to reflect the committed changes.