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.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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.
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 | ||
---|---|---|
109 | Missing blank line after this | |
1074 | 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. |
Thanks Lewis, this looks good to me - just a couple of very minor requested tweaks noted inline.
lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp | ||
---|---|---|
101 | if (FeatureBitStack.empty()) would be a ever so slightly more readable. | |
104 | 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 | Can you add tests for .option push and .option pop with extra tokens at the end of the input? e.g. .option pop 2? |
test/MC/RISCV/option-invalid.s | ||
---|---|---|
23 | Sorry to be pedantic, but you missed a test for .option push with extra tokens at the end. |
if (FeatureBitStack.empty()) would be a ever so slightly more readable.