This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Sync Zvlsseg register order as the same as vector registers.
ClosedPublic

Authored by HsiangKai on Sep 22 2021, 7:06 AM.

Details

Summary

Sync the order of Zvlsseg registers with vector registers to make allocation order consistent with the regular register classes.

This may reduce copies, but we haven't observed it yet.

Diff Detail

Event Timeline

HsiangKai created this revision.Sep 22 2021, 7:06 AM
HsiangKai requested review of this revision.Sep 22 2021, 7:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 22 2021, 7:06 AM

Why do we need updates to tests that don't use segment load/store instructions?

I see, the script was updated to use CHECK-NEXT. Can you pre-commit re-running the script then apply this patch on top?

HsiangKai updated this revision to Diff 374417.Sep 22 2021, 6:46 PM

Precommit mir test update and rebase on that.

I think this is reasonable. I wonder if you have a small test that shows we can avoid copies this way. Unless I missed one case, the updates to the tests only show different registers being used (I understand they're small enough and copies are not a problem for them).

Perhaps you can precommit a test that will have better code generation with this change?

I think this is reasonable. I wonder if you have a small test that shows we can avoid copies this way. Unless I missed one case, the updates to the tests only show different registers being used (I understand they're small enough and copies are not a problem for them).

Perhaps you can precommit a test that will have better code generation with this change?

Sorry for replying late. I have no way to create a small test case to demonstrate it. Register allocator is smart enough to avoid redundant copy.
The redundant copy is found in our internal application.

craig.topper added a comment.EditedOct 6 2021, 9:55 AM

I'm not sure I've seen a test where this removed any copies yet. I asked Kai about the allocation order while investigating an extra copy, but this change did not remove the copy on that test. But I still think it makes sense to allocate registers in a consistent order.

I've updated the commit message to not say that this definitely removes copies.

craig.topper edited the summary of this revision. (Show Details)Oct 6 2021, 9:55 AM

Yeah I can see that a consistent order being the same is probably a good thing, even if we don't see COPYs being removed.

llvm/lib/Target/RISCV/RISCVRegisterInfo.td
400

Are these line breaks intentional?

HsiangKai added inline comments.Oct 7 2021, 1:47 AM
llvm/lib/Target/RISCV/RISCVRegisterInfo.td
400

Yes, I try to group them as (25 ~ 31), (8 ~ 24), (1 ~ 7) after multiply.

I am preparing another patch to reorder the allocation order as v8 to v23, v24 to v31, and v0 to v7. I am not sure if it is better or not. Just refer to GPR that it uses argument registers as the first priority allocation registers. I already forgot why the vector register allocation order starts from v25.

I am preparing another patch to reorder the allocation order as v8 to v23, v24 to v31, and v0 to v7. I am not sure if it is better or not. Just refer to GPR that it uses argument registers as the first priority allocation registers. I already forgot why the vector register allocation order starts from v25.

https://reviews.llvm.org/D111304

I'm not sure I am the best person to accept as I'm not super familiar with anything related to Zvlsseg. Sorry!

craig.topper accepted this revision.Oct 26 2021, 1:30 PM

LGTM other than the comment about adding a comment.

llvm/lib/Target/RISCV/RISCVRegisterInfo.td
391

Probably worth a comment explaining what this is doing.

This revision is now accepted and ready to land.Oct 26 2021, 1:30 PM
HsiangKai updated this revision to Diff 382520.Oct 26 2021, 9:19 PM

Add comments.

This revision was landed with ongoing or failed builds.Oct 27 2021, 10:35 PM
This revision was automatically updated to reflect the committed changes.