[RISCV] Support .option push and .option pop

[lewis-revill]

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.

[asb]

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.

[asb]

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

[lewis-revill]

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

[jrtc27]

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.

[lewis-revill]

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

[asb]

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?

[lewis-revill]

Updated to incorporate suggested changes.

[asb]

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.

[lewis-revill]

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

llvm/trunk/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

 //===-- RISCVAsmParser.cpp - Parse RISCV assembly to MCInst instructions --===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "MCTargetDesc/RISCVAsmBackend.h"
 #include "MCTargetDesc/RISCVMCExpr.h"
 #include "MCTargetDesc/RISCVMCTargetDesc.h"
 #include "MCTargetDesc/RISCVTargetStreamer.h"
 #include "Utils/RISCVBaseInfo.h"
 #include "Utils/RISCVMatInt.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/MC/MCAssembler.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCExpr.h"
 #include "llvm/MC/MCInst.h"
 #include "llvm/MC/MCInstBuilder.h"
 #include "llvm/MC/MCParser/MCAsmLexer.h"
 #include "llvm/MC/MCParser/MCParsedAsmOperand.h"

 // Include the auto-generated portion of the compress emitter.
 #define GEN_COMPRESS_INSTR
 #include "RISCVGenCompressInstEmitter.inc"
 
 namespace {
 struct RISCVOperand;
 
 class RISCVAsmParser : public MCTargetAsmParser {
+  SmallVector<FeatureBitset, 4> FeatureBitStack;
+
   SMLoc getLoc() const { return getParser().getTok().getLoc(); }
   bool isRV64() const { return getSTI().hasFeature(RISCV::Feature64Bit); }
 
   RISCVTargetStreamer &getTargetStreamer() {
     MCTargetStreamer &TS = *getParser().getStreamer().getTargetStreamer();
     return static_cast<RISCVTargetStreamer &>(TS);
   }
 
#include "RISCVGenAsmMatcher.inc"
   void clearFeatureBits(uint64_t Feature, StringRef FeatureString) {
     if (getSTI().getFeatureBits()[Feature]) {
       MCSubtargetInfo &STI = copySTI();
       setAvailableFeatures(
           ComputeAvailableFeatures(STI.ToggleFeature(FeatureString)));
     }
   }
 
+  void pushFeatureBits() {
+    FeatureBitStack.push_back(getSTI().getFeatureBits());
+  }
+
+  bool popFeatureBits() {
+    if (FeatureBitStack.empty())
+      return true;
+
+    FeatureBitset FeatureBits = FeatureBitStack.pop_back_val();
+    copySTI().setFeatureBits(FeatureBits);
+    setAvailableFeatures(ComputeAvailableFeatures(FeatureBits));
+
+    return false;
+  }
 public:
   enum RISCVMatchResultTy {
     Match_Dummy = FIRST_TARGET_MATCH_RESULT_TY,
 #define GET_OPERAND_DIAGNOSTIC_TYPES
 #include "RISCVGenAsmMatcher.inc"
 #undef GET_OPERAND_DIAGNOSTIC_TYPES
   };
 
bool RISCVAsmParser::parseDirectiveOption() {
   AsmToken Tok = Parser.getTok();
   // At the moment only identifiers are supported.
   if (Tok.isNot(AsmToken::Identifier))
     return Error(Parser.getTok().getLoc(),
                  "unexpected token, expected identifier");
 
   StringRef Option = Tok.getIdentifier();
 
+  if (Option == "push") {
+    getTargetStreamer().emitDirectiveOptionPush();
+
+    Parser.Lex();
+    if (Parser.getTok().isNot(AsmToken::EndOfStatement))
+      return Error(Parser.getTok().getLoc(),
+                   "unexpected token, expected end of statement");
+
+    pushFeatureBits();
+    return false;
+  }
+
+  if (Option == "pop") {
+    SMLoc StartLoc = Parser.getTok().getLoc();
+    getTargetStreamer().emitDirectiveOptionPop();
+
+    Parser.Lex();
+    if (Parser.getTok().isNot(AsmToken::EndOfStatement))
+      return Error(Parser.getTok().getLoc(),
+                   "unexpected token, expected end of statement");
+
+    if (popFeatureBits())
+      return Error(StartLoc, ".option pop with no .option push");
+
+    return false;
+  }
+
   if (Option == "rvc") {
     getTargetStreamer().emitDirectiveOptionRVC();
 
     Parser.Lex();
     if (Parser.getTok().isNot(AsmToken::EndOfStatement))
       return Error(Parser.getTok().getLoc(),
                    "unexpected token, expected end of statement");
 
if (Parser.getTok().isNot(AsmToken::EndOfStatement))
                    "unexpected token, expected end of statement");
 
     clearFeatureBits(RISCV::FeatureRelax, "relax");
     return false;
   }
 
   // Unknown option.
   Warning(Parser.getTok().getLoc(),
-          "unknown option, expected 'rvc', 'norvc', 'relax' or 'norelax'");
+          "unknown option, expected 'push', 'pop', 'rvc', 'norvc', 'relax' or "
+          "'norelax'");
   Parser.eatToEndOfStatement();
   return false;
 }
 
 void RISCVAsmParser::emitToStreamer(MCStreamer &S, const MCInst &Inst) {
   MCInst CInst;
   bool Res = compressInst(CInst, Inst, getSTI(), S.getContext());
   CInst.setLoc(Inst.getLoc());

llvm/trunk/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.h

 
 namespace llvm {
 
 class RISCVTargetELFStreamer : public RISCVTargetStreamer {
 public:
   MCELFStreamer &getStreamer();
   RISCVTargetELFStreamer(MCStreamer &S, const MCSubtargetInfo &STI);
 
+  virtual void emitDirectiveOptionPush();
+  virtual void emitDirectiveOptionPop();
   virtual void emitDirectiveOptionRVC();
   virtual void emitDirectiveOptionNoRVC();
   virtual void emitDirectiveOptionRelax();
   virtual void emitDirectiveOptionNoRelax();
 };
 }
 #endif

llvm/trunk/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

RISCVTargetELFStreamer::RISCVTargetELFStreamer(MCStreamer &S,
 
   MCA.setELFHeaderEFlags(EFlags);
 }
 
 MCELFStreamer &RISCVTargetELFStreamer::getStreamer() {
   return static_cast<MCELFStreamer &>(Streamer);
 }
 
+void RISCVTargetELFStreamer::emitDirectiveOptionPush() {}
+void RISCVTargetELFStreamer::emitDirectiveOptionPop() {}
 void RISCVTargetELFStreamer::emitDirectiveOptionRVC() {}
 void RISCVTargetELFStreamer::emitDirectiveOptionNoRVC() {}
 void RISCVTargetELFStreamer::emitDirectiveOptionRelax() {}
 void RISCVTargetELFStreamer::emitDirectiveOptionNoRelax() {}

llvm/trunk/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.h

 #include "llvm/MC/MCStreamer.h"
 
 namespace llvm {
 
 class RISCVTargetStreamer : public MCTargetStreamer {
 public:
   RISCVTargetStreamer(MCStreamer &S);
 
+  virtual void emitDirectiveOptionPush() = 0;
+  virtual void emitDirectiveOptionPop() = 0;
   virtual void emitDirectiveOptionRVC() = 0;
   virtual void emitDirectiveOptionNoRVC() = 0;
   virtual void emitDirectiveOptionRelax() = 0;
   virtual void emitDirectiveOptionNoRelax() = 0;
 };
 
 // This part is for ascii assembly output
 class RISCVTargetAsmStreamer : public RISCVTargetStreamer {
   formatted_raw_ostream &OS;
 
 public:
   RISCVTargetAsmStreamer(MCStreamer &S, formatted_raw_ostream &OS);
 
+  void emitDirectiveOptionPush() override;
+  void emitDirectiveOptionPop() override;
   void emitDirectiveOptionRVC() override;
   void emitDirectiveOptionNoRVC() override;
   void emitDirectiveOptionRelax() override;
   void emitDirectiveOptionNoRelax() override;
 };
 
 }
 #endif

llvm/trunk/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp

 
 RISCVTargetStreamer::RISCVTargetStreamer(MCStreamer &S) : MCTargetStreamer(S) {}
 
 // This part is for ascii assembly output
 RISCVTargetAsmStreamer::RISCVTargetAsmStreamer(MCStreamer &S,
                                                formatted_raw_ostream &OS)
     : RISCVTargetStreamer(S), OS(OS) {}
 
+void RISCVTargetAsmStreamer::emitDirectiveOptionPush() {
+  OS << "\t.option\tpush\n";
+}
+
+void RISCVTargetAsmStreamer::emitDirectiveOptionPop() {
+  OS << "\t.option\tpop\n";
+}
+
 void RISCVTargetAsmStreamer::emitDirectiveOptionRVC() {
   OS << "\t.option\trvc\n";
 }
 
 void RISCVTargetAsmStreamer::emitDirectiveOptionNoRVC() {
   OS << "\t.option\tnorvc\n";
 }
 
 void RISCVTargetAsmStreamer::emitDirectiveOptionRelax() {
   OS << "\t.option\trelax\n";
 }
 
 void RISCVTargetAsmStreamer::emitDirectiveOptionNoRelax() {
   OS << "\t.option\tnorelax\n";
 }

llvm/trunk/test/MC/RISCV/option-invalid.s

 # RUN: not llvm-mc -triple riscv32 < %s 2>&1 \
 # RUN: | FileCheck -check-prefixes=CHECK %s
 
 # CHECK: error: unexpected token, expected identifier
 .option
 
 # CHECK: error: unexpected token, expected identifier
 .option 123
 
 # CHECK: error: unexpected token, expected identifier
 .option "str"
 
 # CHECK: error: unexpected token, expected end of statement
 .option rvc foo
 
-# CHECK: warning: unknown option, expected 'rvc', 'norvc', 'relax' or 'norelax'
+# CHECK: warning: unknown option, expected 'push', 'pop', 'rvc', 'norvc', 'relax' or 'norelax'
 .option bar
+
+# CHECK: error: .option pop with no .option push
+.option pop
+
+# CHECK: error: unexpected token, expected end of statement
+.option push 123
+
+# CHECK: error: unexpected token, expected end of statement
+.option pop 123

llvm/trunk/test/MC/RISCV/option-pushpop.s

+# RUN: llvm-mc -triple riscv32 -mattr=-relax -riscv-no-aliases < %s \
+# RUN:     | FileCheck -check-prefix=CHECK-INST %s
+# RUN: llvm-mc -filetype=obj -triple riscv32 < %s \
+# RUN:     | llvm-readobj -r | FileCheck -check-prefix=CHECK-RELOC %s
+# RUN: llvm-mc -triple riscv32 -filetype=obj < %s \
+# RUN:     | llvm-objdump  -triple riscv32 -mattr=+c -d - \
+# RUN:     | FileCheck -check-prefixes=CHECK-BYTES,CHECK-ALIAS %s
+
+# RUN: llvm-mc -triple riscv64 -mattr=-relax -riscv-no-aliases < %s \
+# RUN:     | FileCheck -check-prefix=CHECK-INST %s
+# RUN: llvm-mc -filetype=obj -triple riscv64 < %s \
+# RUN:     | llvm-readobj -r | FileCheck -check-prefix=CHECK-RELOC %s
+# RUN: llvm-mc -triple riscv64 -filetype=obj < %s \
+# RUN:     | llvm-objdump  -triple riscv64 -mattr=+c -d - \
+# RUN:     | FileCheck -check-prefixes=CHECK-BYTES,CHECK-ALIAS %s
+
+# Test the operation of the push and pop assembler directives when
+# using .option relax and .option rvc. Checks that using .option pop
+# correctly restores all target features to their state at the point
+# where .option pop was last used.
+
+# CHECK-INST: call foo
+# CHECK-RELOC: R_RISCV_CALL foo 0x0
+# CHECK-RELOC-NOT: R_RISCV_RELAX foo 0x0
+call foo
+
+# CHECK-INST: addi s0, sp, 1020
+# CHECK-BYTES: 13 04 c1 3f
+# CHECK-ALIAS: addi s0, sp, 1020
+addi s0, sp, 1020
+
+.option push    # Push relax=false, rvc=false
+# CHECK-INST: .option push
+
+.option relax
+# CHECK-INST: .option relax
+# CHECK-INST: call bar
+# CHECK-RELOC-NEXT: R_RISCV_CALL bar 0x0
+# CHECK-RELOC-NEXT: R_RISCV_RELAX bar 0x0
+call bar
+
+.option push    # Push relax=true, rvc=false
+# CHECK-INST: .option push
+
+.option rvc
+# CHECK-INST: .option rvc
+# CHECK-INST: c.addi4spn s0, sp, 1020
+# CHECK-BYTES: e0 1f
+# CHECK-ALIAS: addi s0, sp, 1020
+addi s0, sp, 1020
+
+.option pop     # Pop relax=true, rvc=false
+# CHECK-INST: .option pop
+# CHECK-INST: addi s0, sp, 1020
+# CHECK-BYTES: 13 04 c1 3f
+# CHECK-ALIAS: addi s0, sp, 1020
+addi s0, sp, 1020
+
+# CHECK-INST: call bar
+# CHECK-RELOC-NEXT: R_RISCV_CALL bar 0x0
+# CHECK-RELOC-NEXT: R_RISCV_RELAX bar 0x0
+call bar
+
+.option pop     # Pop relax=false, rvc=false
+# CHECK-INST: .option pop
+# CHECK-INST: call baz
+# CHECK-RELOC: R_RISCV_CALL baz 0x0
+# CHECK-RELOC-NOT: R_RISCV_RELAX baz 0x0
+call baz
+
+# CHECK-INST: addi s0, sp, 1020
+# CHECK-BYTES: 13 04 c1 3f
+# CHECK-ALIAS: addi s0, sp, 1020
+addi s0, sp, 1020