Page MenuHomePhabricator

[RISCV] Support for mapping symbol in RISCV.
Needs ReviewPublic

Authored by LiDongjin on Nov 4 2022, 5:38 AM.

Details

Summary

Support for mapping symbol in RISCV.

There are three types in mapping symbols: $d $x $x<isa>.
For $d and $x is same as ARM, So i just copy related codes and testcases
For $x<isa>, the content of isa must same as Tag_RISCV_arch.
When emit Tag_RISCV_arch attribute, The following codes must have $x<isa> mapping symbol.
Adjacent same Tag_RISCV_arch attribute do not need two$x<isa>.

Here we have two testcases: mapping-symbols.s and mapping-isa.s.
mapping-symbols.s about the $d and $x in different segment.
mapping-isa.s about $x<isa> in different attribute Tag_RISCV_arch.

The mapping symbol in riscv references:
https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/196

Diff Detail

Event Timeline

LiDongjin created this revision.Nov 4 2022, 5:38 AM
Herald added a reviewer: MaskRay. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
LiDongjin requested review of this revision.Nov 4 2022, 5:38 AM
LiDongjin edited the summary of this revision. (Show Details)Nov 4 2022, 5:43 AM
LiDongjin removed reviewers: jhenderson, MaskRay.
LiDongjin removed subscribers: StephenFan, eopXD, pcwang-thead and 33 others.
LiDongjin retitled this revision from [RISCV] add mapping symbol to RISCV ELF. to [WIP] add mapping symbol to RISCV ELF..Nov 4 2022, 5:53 AM
LiDongjin retitled this revision from [WIP] add mapping symbol to RISCV ELF. to [WIP] [RISCV] add mapping symbol to RISCV ELF..
LiDongjin retitled this revision from [WIP] [RISCV] add mapping symbol to RISCV ELF. to [WIP] [RISCV] Support for mapping symbol in RISCV。.Nov 4 2022, 5:59 AM
LiDongjin updated this revision to Diff 473498.Nov 6 2022, 6:57 AM
LiDongjin retitled this revision from [WIP] [RISCV] Support for mapping symbol in RISCV。 to [WIP] [RISCV] Support for mapping symbol in RISCV..
LiDongjin edited the summary of this revision. (Show Details)
StephenFan added inline comments.Nov 6 2022, 7:17 PM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
371

The comment line should be deleted.

409

These comments should be deleted.

llvm/test/MC/RISCV/mapping-isa.s
2

Copy-paste from ARM?

StephenFan added inline comments.Nov 6 2022, 7:20 PM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
27

Do we need this header file?

LiDongjin updated this revision to Diff 473550.Nov 6 2022, 8:31 PM
LiDongjin edited the summary of this revision. (Show Details)
LiDongjin marked 4 inline comments as done.
LiDongjin edited the summary of this revision. (Show Details)Nov 6 2022, 9:45 PM
LiDongjin edited the summary of this revision. (Show Details)
LiDongjin retitled this revision from [WIP] [RISCV] Support for mapping symbol in RISCV. to [RISCV] Support for mapping symbol in RISCV..Nov 7 2022, 12:33 AM
LiDongjin edited the summary of this revision. (Show Details)Nov 7 2022, 12:47 AM
StephenFan added inline comments.Nov 7 2022, 7:00 AM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
118

It seems like 5 is a magic number.

LiDongjin added inline comments.Nov 7 2022, 6:03 PM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
118

It will change to RISCVAttrs::ARCH.

asb added a comment.Nov 8 2022, 2:40 AM

@kito-cheng, @jrtc27: I was wondering on if you could comment from a psABI workgroup perspective on the status of the mapping symbol proposal and the path to it becoming part of the standard?

The short summary is that there was a bunch of discussion about a year ago, things went quiet and nobody pushed it forward to get consensus and land it in the spec. I don't think there are outstanding technical reasons for that though, just that it wasn't prioritised and followed up on. Even if we did want to change it we're probably stuck with the current draft, anyway, since an implementation of it was merged in binutils a while ago despite that being bad practice for ABI things...

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
189–190

Can the moving of this please be split out into a separate patch? That makes it obvious what's changing and what's just moving.

kito-cheng added inline comments.Dec 16 2022, 7:51 AM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
220

"$x" + LastEMSInfo->ISAInfo + "", we didn't use < and > around arch string

LiDongjin updated this revision to Diff 487046.Fri, Jan 6, 10:05 PM
LiDongjin added inline comments.Fri, Jan 6, 10:06 PM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
118

Done

220

Done

LiDongjin updated this revision to Diff 487053.Fri, Jan 6, 10:59 PM
LiDongjin updated this revision to Diff 487059.Fri, Jan 6, 11:53 PM

LGTM, this match what we have in psABI PR and binutils implemention, but I would like to make sure @asb is happy with this too :)

asb added a comment.Tue, Jan 24, 7:31 AM

LGTM, this match what we have in psABI PR and binutils implemention, but I would like to make sure @asb is happy with this too :)

I've not done a super in-depth review, but from a high-level look it seems good to me. Despite the fact binutils already supports this, I'd probably rather see the psABI change approved before landing this patch. Is it possible that might happen soon?

LGTM, this match what we have in psABI PR and binutils implemention, but I would like to make sure @asb is happy with this too :)

I've not done a super in-depth review, but from a high-level look it seems good to me. Despite the fact binutils already supports this, I'd probably rather see the psABI change approved before landing this patch. Is it possible that might happen soon?

Going to push this forward, hope this will happen soon, will notify here once it getting merged :)