This is an archive of the discontinued LLVM Phabricator instance.

[MC][RISCV] Add RISCV MCObjectFileInfo
ClosedPublic

Authored by flip1995 on May 7 2021, 12:58 AM.

Details

Summary

This makes sure, that the text section will have a 2-byte alignment, if
the +c extension is enabled.

Diff Detail

Event Timeline

flip1995 created this revision.May 7 2021, 12:58 AM

The banners were copied from other files but not updated to match the new file.

IIRC no one was opposed to this alignment change when we discussed it in a recent RISC-V sync-up call, but let's give it some time for more people to chime in.

llvm/test/MC/RISCV/align.s
41–44

enable -> enabled

flip1995 updated this revision to Diff 347640.May 25 2021, 5:03 AM
flip1995 marked an inline comment as done.

rebased

flip1995 published this revision for review.May 25 2021, 5:05 AM
flip1995 added reviewers: MaskRay, asb.
Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2021, 5:05 AM
MaskRay added inline comments.May 25 2021, 10:40 AM
llvm/lib/MC/MCObjectFileInfo.cpp
984

{}

= default doesn't change the fact that it is user-defined. IMHO {} is the prevailing style.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCObjectFileInfo.cpp
22

Use just one variable. Don't keep Ctx

24

Drop braces around simple statements.

Just use ? :

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCObjectFileInfo.h
20

Is this used?

flip1995 updated this revision to Diff 348929.Jun 1 2021, 3:58 AM

addressed review comments and rebased

flip1995 updated this revision to Diff 348930.Jun 1 2021, 4:01 AM

Fixed clang-tidy error

riscv64-linux-gnu-as -march=rv64gc a.s does use 2-byte alignment, so I think this may be fine.

I just cannot find sufficient justification for such a change.

I just cannot find sufficient justification for such a change.

So besides getting the same behavior between GCC and LLVM, this change also improves code size for RISC-V. This is because there doesn't have to be padding added between functions, if they aren't 4 byte aligned.

Ping other reviewers. Personally I think the necessity is unclear.

asb added a comment.Aug 19 2021, 5:53 AM

I'd be inclined to accept this on the basis that wherever it's not overly burdensome I've typically tried to match our output to GCC (mainly to avoid wasting time looking at irrelevant differences), and the change itself seems uncomplicated.

@flip1995: thanks for your patience, could you please rebase as this doesn't apply cleanly against current main.

If anyone has strong concerns about landing this, please speak up now!

flip1995 marked 4 inline comments as done.Aug 23 2021, 5:58 AM
MaskRay accepted this revision.Aug 26 2021, 8:06 PM

LGTM.

This revision is now accepted and ready to land.Aug 26 2021, 8:06 PM
flip1995 updated this revision to Diff 369041.Aug 27 2021, 1:11 AM

Rebased + Formatted

luismarques accepted this revision.Aug 27 2021, 1:37 AM
This revision was automatically updated to reflect the committed changes.