This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][NFC] Use uncompressInst to relax instructions
ClosedPublic

Authored by pcwang-thead on Jan 16 2023, 2:28 AM.

Details

Summary

As the TODO said, we can just use generated uncompressInst to
relax instructions.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2023, 2:28 AM
pcwang-thead requested review of this revision.Jan 16 2023, 2:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2023, 2:28 AM
kito-cheng accepted this revision.Jan 16 2023, 7:17 AM

LGTM with a nit, seems like it also improve the debug info (loc).

llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
178–180

nit: Assert check return value of uncompressInst.

This revision is now accepted and ready to land.Jan 16 2023, 7:17 AM

Address comment.

pcwang-thead marked an inline comment as done.Jan 16 2023, 7:13 PM

Can we do this in a way that doesn't replicate the uncompressInst function in two different cpp files.

kito-cheng added inline comments.Jan 16 2023, 10:58 PM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
179

Another nit, you need add (void)Success; to suppress unused variable warning for disable assertion build.

Address comments.

pcwang-thead marked an inline comment as done.Jan 17 2023, 12:05 AM
craig.topper added inline comments.Jan 17 2023, 12:09 AM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
178–180

Where is RISCRVC defined?

pcwang-thead added a comment.EditedJan 17 2023, 12:11 AM

Can we do this in a way that doesn't replicate the uncompressInst function in two different cpp files.

Thanks, I have thought about it too yesterday. :)
So I moved compressInst/uncompressInst to RISCVBaseInfo (D141897) and removed all other duplicated code.
Additionally, because there are redefinition errors of RISCVValidateMCOperand, so I added postfix to these validators (D141896).

pcwang-thead marked an inline comment as done.Jan 17 2023, 12:14 AM
craig.topper added inline comments.Jan 17 2023, 12:14 AM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
178–180

Never mind I didn’t see the other patch

asb added a comment.Jan 17 2023, 6:45 AM

LGTM, thanks for doing this cleanup.

I've posted https://reviews.llvm.org/D141951 to removed the uncompressInst's dependency on MCRegisterInfo which should simplify this patch.

I've posted https://reviews.llvm.org/D141951 to removed the uncompressInst's dependency on MCRegisterInfo which should simplify this patch.

Thanks! It's much more concise now. :-)

This revision was landed with ongoing or failed builds.Jan 17 2023, 10:34 PM
This revision was automatically updated to reflect the committed changes.