This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Support .option rvc and norvc
ClosedPublic

Authored by kito-cheng on Apr 19 2018, 11:51 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

kito-cheng created this revision.Apr 19 2018, 11:51 PM
apazos added inline comments.Apr 20 2018, 11:13 AM
test/MC/RISCV/option-rvc.s
22 ↗(On Diff #143232)

Kito, can you add an instruction before the first .option rvc to make sure the default behavior is unaltered:
if C ext is enabled, it will compress, but if it is disable it will not compress.

apazos added inline comments.Apr 20 2018, 5:52 PM
test/MC/RISCV/option-rvc.s
29 ↗(On Diff #143232)

and add more than one instruction in an option block to show it compresses or not all of instructions in the block.

sabuasal added inline comments.Apr 22 2018, 2:52 PM
lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1000 ↗(On Diff #143232)

no braces needed.

1012 ↗(On Diff #143232)

no braces needed

1023 ↗(On Diff #143232)

no braces needed

1035 ↗(On Diff #143232)

no braces needed

sabuasal added inline comments.Apr 22 2018, 3:16 PM
lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.h
21 ↗(On Diff #143232)

This should be pure virtual method that you only implement in your RISCVTsargetELFstreamer and your RISCVASMstreamer. Making it virtual causes extra entries for your vtable.

Look at MCStreamer.h line 62

Changes:

  • Fix coding style issue.
  • Add more instructions to testcase.
  • Make emitDirectiveOptionRVC and emitDirectiveOptionNoRVC to pure virtual function.
  • Return nullptr rather than RISCVTargetStreamer in createRISCVObjectTargetStreamer, because we have pure virtual function in RISCVTargetStreamer now.
kito-cheng marked 7 inline comments as done.Apr 24 2018, 12:48 AM
asb added a comment.Apr 25 2018, 8:11 AM

Thanks Kito. Could you please add some tests for the new warning/failure cases introduced by this patch? e.g. .option with unrecognised identifier etc.

lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp
98–99 ↗(On Diff #143682)

Unnecessary comment - the function call is pretty self-descriptive.

kito-cheng updated this revision to Diff 144828.May 1 2018, 8:22 PM

Changes:

  • Add testcase for warning/failure cases.
asb accepted this revision.May 3 2018, 6:19 AM

Thanks for adding the test case, this looks good to me now. Final nit: could youplease get rid of the unnecessary comment for RegisterAsmTargetStreamer (https://reviews.llvm.org/D45864#inline-403118)?

This revision is now accepted and ready to land.May 3 2018, 6:19 AM
asb added a comment.May 3 2018, 7:32 AM

There's a corner case we're probably going to encounter here vs gcc.

In GCC, .option rvc or .option norvc in inline asm will have the effect of enabling/disabling instruction compression from that point onwards. I assume it won't have that effect with LLVM unless you use -save-temps. It might be that we decide this use of inline asm isn't supported by LLVM and people should use appropriate function or module attributes instead. Either way, a test which demonstrates the behaviour would be worthwhile.

asb requested changes to this revision.May 3 2018, 7:34 AM
This revision now requires changes to proceed.May 3 2018, 7:34 AM

Changes:

  • Add 2 test cases to show .option rvc and .option norvc in inline asm will not effect the code gen.
asb accepted this revision.May 11 2018, 10:29 AM

Thanks Kito! I'll add a comment to the test/CodeGen/RISCV files when committing just to indicate their purpose.

This revision is now accepted and ready to land.May 11 2018, 10:29 AM
This revision was automatically updated to reflect the committed changes.