Page MenuHomePhabricator

[RISCV] Encode RISCV specific ELF e_flags to RISCV Binary by RISCVTargetStreamer
ClosedPublic

Authored by shiva0217 on Jan 1 2018, 10:27 PM.

Details

Reviewers
asb
apazos
Summary

Encode RISCV specific ELF e_flag EF_RISCV_RVC to RISCV Binary

Diff Detail

Repository
rL LLVM

Event Timeline

shiva0217 created this revision.Jan 1 2018, 10:27 PM
asb added a comment.Jan 2 2018, 6:19 AM

Thanks Shiva - comments inline

lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
15–25

A number of these seem to be unused.

45–48

I don't think this logic is doing what we want. The ABI EFLAGs should be determined by the target ABI rather than the target architecture. I'd suggest removing the FLOAT_ABI code and tests, and we can add this logic once alternative -target-abi are supported.

lib/Target/RISCV/RISCVTargetStreamer.h
1 ↗(On Diff #128396)

Shouldn't this file be in lib/Target/RISCV/MCTargetDesc?

13–17 ↗(On Diff #128396)

Most of these seem to be unused.

test/CodeGen/RISCV/elf-header.ll
1 ↗(On Diff #128396)

Doesn't this really belong as a test/MC/RISCV test? (obviously modified to use llvm-mc instead).

shiva0217 updated this revision to Diff 128484.Jan 2 2018, 6:41 PM
shiva0217 added inline comments.Jan 2 2018, 6:48 PM
lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
15–25

Remove unused including files.

45–48

I think this should be reasonable default ABI EFLAGs setting before introducing -target-abi flag. OK, we could add the logic once -target-abi are supported.

lib/Target/RISCV/RISCVTargetStreamer.h
1 ↗(On Diff #128396)

Move RISCVTargetStreamer.h to lib/Target/RISCV/MCTargetDesc

13–17 ↗(On Diff #128396)

Remove unused including files.

test/CodeGen/RISCV/elf-header.ll
1 ↗(On Diff #128396)

Move the test case to test/MC/RISCV and rename to elf-flags.s

asb added a comment.Jan 11 2018, 8:28 AM

With a couple of minor nits, looks good to me. Thanks as always Shiva

lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.h
23

Nitpick: full-stop after comment.

Does the comment add value? Maybe just delete it.

test/MC/RISCV/elf-flags.s
2

Need to set the target for llvm-mc.

Should probably have RUN lines for both riscv32 and riscv64 as many of the other test/MC/RISCV tests do.

shiva0217 added inline comments.Jan 11 2018, 10:38 PM
lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.h
23

Remove the redundant comment

test/MC/RISCV/elf-flags.s
2

Run the test case for riscv32 and riscv64 target.

asb added a comment.Jan 12 2018, 12:45 AM

Hi Shiva. I was just looking at applying this and the STI field is triggering a compiler warning as it's not used: it's initialised in the RISCVTargetELFStreamer constructor, but isn't actually read from anywhere.

The test could be modified slightly - given that we know all defined ELF flags for RISC-V are shared between RV32 and RV64 we can share check line, and we could also check that the RVC flag isn't improperly being when not targeting RVC. How about writing it like this:

# RUN: llvm-mc -triple=riscv32 -filetype=obj < %s | llvm-readobj -file-headers - | FileCheck -check-prefixes=CHECK-RVI %s
# RUN: llvm-mc -triple=riscv64 -filetype=obj < %s | llvm-readobj -file-headers - | FileCheck -check-prefixes=CHECK-RVI %s
# RUN: llvm-mc -triple=riscv32 -mattr=+c -filetype=obj < %s | llvm-readobj -file-headers - | FileCheck -check-prefixes=CHECK-RVIC %s
# RUN: llvm-mc -triple=riscv64 -mattr=+c -filetype=obj < %s | llvm-readobj -file-headers - | FileCheck -check-prefixes=CHECK-RVIC %s

# CHECK-RVI:       Flags [ (0x0)
# CHECK-RVI-NEXT:  ]

# CHECK-RVIC:       Flags [ (0x1)
# CHECK-RVIC-NEXT:    EF_RISCV_RVC (0x1)
# CHECK-RVIC-NEXT:  ]

nop
asb added a comment.Jan 12 2018, 12:46 AM

My suggestion would be to remove the MCSubtarget field (it can be reintroduced when future changes need it), but obviously keep it as an argument to the constructor where it is used.

shiva0217 updated this revision to Diff 129612.Jan 12 2018, 6:09 AM
shiva0217 edited the summary of this revision. (Show Details)

Remove MCSubtargetInfo field and refine the test case as @asb suggest.

apazos added inline comments.Jan 12 2018, 1:00 PM
lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp
74

Should'nt we check for ELF format, that is the only one supported now, but may change later?

const Triple &TT = STI.getTargetTriple();

if (TT.isOSBinFormatELF())
  return new RISCVTargetELFStreamer(S);
return nullptr;
89

Reading MCStreamer.h I understand when we define a target streamer we need to define two streamers, ELF and Asm. Is it how you interpreted?

/ Target specific streamer interface. This is used so that targets can
/ implement support for target specific assembly directives.
/
/ If target foo wants to use this, it should implement 3 classes:
/ * FooTargetStreamer : public MCTargetStreamer
/ * FooTargetAsmStreamer : public FooTargetStreamer
/// * FooTargetELFStreamer : public FooTargetStreamer

lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
42

Shouldn't it be return static_cast<RISCVELFStreamer &>(Streamer);

shiva0217 updated this revision to Diff 129796.Jan 14 2018, 9:27 PM
shiva0217 added inline comments.Jan 14 2018, 9:57 PM
lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp
74

Add ELF format checking before new RISCVTargetELFStreamer.

89

To my understanding, AsmStreamer is used to emit target specific assembly directives,
E.g. ARM emit .fpu directive by AsmStreamer and encode the information to a section by ELFStreamer. It seems that RISCV GCC will not emit directive to specify target feature(E.g. C EXT ISA), so I define ELFStreamer first. I think we could introduce the AsmStreamer in the future patch when we need to emit assembly directives.

lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
42

We didn't define RISCVELFStreamer class for emitting e_flags. It seems that we could introduce the class in the feature patch?

asb accepted this revision.Jan 17 2018, 6:34 AM

clang-format shows me a few minor tweaks:

  • RISCVELFStreamer.h include should come first in RISCVELFStreamer.cpp
  • RISCVMCAsmInfo.h include is now improperly sorted in RISCVMCTargetDesc.cpp
  • RISCVTargetStreamer constructor can fit on one line in 80 chars RISCVTargetStreamer::RISCVTargetStreamer(MCStreamer &S) : MCTargetStreamer(S) {}

Otherwise it looks good to me. I can't see any hard dependency on a custom RISCVAsmStreamer (though it's likely we'll want to add it later). Ana - are you happy for this patch to be committed as-is?

This revision is now accepted and ready to land.Jan 17 2018, 6:34 AM

Thanks, this looks fine to me. You can merge it. I will rebase my change on top of this.

asb added a comment.Jan 25 2018, 9:59 AM

Hi Shiva, why don't you follow the instructions at http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access and request commit access? Many many thanks again for all of your contributions to the RISC-V backend so far.

Hi Alex, thanks for the advice. Hi Ana, I have committed the patch to the trunk.

asb added a comment.Jan 26 2018, 12:39 AM

Hi Shiva. In the future it's worth nothing that if you include the review url in the commit, then Phabricator automatically picks it up.

e.g. if you included this line then phabricator would automatically close this review:
Differential Revision: https://reviews.llvm.org/D41658

It looks like you missed the minor sorting/whitespace fixes I suggested https://reviews.llvm.org/D41658#978491. If you have time to commit a follow-up cleanup I'd appreciate it (just committing directly for this cleanup is fine). Thanks!

asb added a comment.Jan 26 2018, 12:43 AM

Sorry, my mistake - the committed version did include the suggested changes.

Hi Alex. Thanks for your tips, I'll remember that.

shiva0217 closed this revision.Jan 28 2018, 5:52 PM

Closed by commit rL323507: [RISCV] Encode RISCV specific ELF e_flags to RISCV Binary by RISCVTargetStreamer