Page MenuHomePhabricator

[RISCV] Support .option push and .option pop
ClosedPublic

Authored by lewis-revill on May 4 2018, 4:09 AM.

Details

Summary

This adds support in the RISCVAsmParser the storing of Subtarget feature bits to a stack so that they can be pushed/popped to enable/disable multiple features at once.

Diff Detail

Repository
rL LLVM

Event Timeline

simoncook created this revision.May 4 2018, 4:09 AM
asb requested changes to this revision.May 16 2018, 6:57 AM

Could you please expand the test to verify push+pop works for a pair of options (rvc+relax)? Passing -riscv-no-aliases should let you check whether a compressed instruction was produced or not.

This revision now requires changes to proceed.May 16 2018, 6:57 AM
asb added a comment.Aug 15 2018, 7:45 AM

Do you think you might have a chance to update this patch?

lewis-revill commandeered this revision.Oct 19 2018, 3:36 AM
lewis-revill added a reviewer: simoncook.

I've rebased this patch and also added the case where relax+rvc are enabled to the testcase.

A couple of minor comments but otherwise looks good to my non-authoritative eyes.

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
136 ↗(On Diff #170179)

Missing blank line after this

1325 ↗(On Diff #170179)

You use StartLoc here but manually call Parser.geTok().getLoc() earlier. Personally I'd just remove StartLoc altogether, it's only required on the error paths and only ever used once on each path.

Thank you for your comments, I've addressed them in this update.

lewis-revill marked 2 inline comments as done.Oct 24 2018, 8:18 AM
asb added a comment.Nov 12 2018, 6:40 AM

Thanks Lewis, this looks good to me - just a couple of very minor requested tweaks noted inline.

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
128 ↗(On Diff #170897)

if (FeatureBitStack.empty()) would be a ever so slightly more readable.

131 ↗(On Diff #170897)

LLVM is quite conservative about the use of auto, though this isn't always adhered to throughout LLVM https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable. Might be worth using an explicit type here.

test/MC/RISCV/option-invalid.s
18 ↗(On Diff #170897)

Can you add tests for .option push and .option pop with extra tokens at the end of the input? e.g. .option pop 2?

Updated to incorporate suggested changes.

lewis-revill marked 3 inline comments as done.Nov 12 2018, 8:44 AM
asb added inline comments.Nov 13 2018, 8:14 AM
test/MC/RISCV/option-invalid.s
23 ↗(On Diff #173688)

Sorry to be pedantic, but you missed a test for .option push with extra tokens at the end.

Added missed test line for .option push with extra tokens.

asb accepted this revision.Nov 28 2018, 8:41 AM
This revision is now accepted and ready to land.Nov 28 2018, 8:41 AM
This revision was automatically updated to reflect the committed changes.