[RISCV] Support .option relax and .option norelax

[lewis-revill]

Summary

This extends the .option support from D45864 to enable/disable the relax feature flag from D44886

During parsing of the relax/norelax directives, the RISCV::FeatureRelax feature bits of the SubtargetInfo stored in the AsmParser are updated appropriately to reflect whether relaxation is currently enabled in the parser. When an instruction is parsed, the parser checks if relaxation is currently enabled and if so, gets a handle to the AsmBackend and sets the ForceRelocs flag. The AsmBackend uses a combination of the original RISCV::FeatureRelax feature bits set by e.g -mattr=+/-relax and the ForceRelocs flag to determine whether to emit relocations for symbol and branch diffs. Diff relocations should therefore only not be emitted if the relax flag was not set on the command line and no instruction was ever parsed in a section with relaxation enabled to ensure correct diffs are emitted.

[asb]

I think it would be worth expanding option-relax.s to also check symbol diffing does/doesn't produce relocations for .option relax/norelax. Other than that, this is looking good to me.

[simoncook]

Note: I've marked this as WIP whilst discussing the interface, I'm not suggesting hardcoding in a setSTI() function to MCAsmBackend, that will be removed from the final patch

I added a test to check the relax feature affects symbol differences as we would expect, and it didn't have the intended effect. The issue is that the MCAsmBackend is created with a STI that is created before we start parsing assembly, and that when we parse .option relax/norelax/rvc/norvc, we create a new STI, but this remains local to the AsmParser, but the MCAsmBackend keeps the original. To compare, what happens for instructions, our MatchInstructionImpl is given the current STI, so has the default flags, so enabling/disable works as expected.

What I've done here is when we parse relax/norelax I get a handle on the AsmBackend and update a pointer to the current STI that should be used. (Currently this is clumsily pushed into the AsmBackend interface, if we go down this route I'll make this a target specific hook). This way when we go to emit the constant we know whether the relax flag is set.

This solves half the issue, the other is the issue about only having the last state available (described https://reviews.llvm.org/D45181#1086541), as expressions will be converted to relocation once the entire file is parsed, and there is nowhere for extra metadata that a particular expression should be relaxed.

I've given it some thought, and I think if we want to have these relocations be optional, then I think storing a flag in AsmBackend to switch on the relocations for all symbol differences makes more sense than the broken state if things are switched off. So setting .option relax would set this flag, but .option norelax wouldn't unset it, and the logic for requiresDiffExpressionRelocations would check STI and this flag, and if either were set this would return true. I think this seems a better user experience than having a warning, or letting this just break.

Any thoughts?

[asb]

I'd still err towards .option norelax meaning "do what I say". If a user freely intermixes .option relax and .option norelax within a section then there's a whole bunch of things that might break. Simply ignoring the norelax doesn't seem right, and picking out particular cases (e.g. symbol diffs) where we'll act as if relaxation was enabled also seems a bit dodgy. I still think that erroring might be safest and simplest path.

[asb]

Are you planning on updating this? I still feel that either producing an error, or doing what the user says and honouring .option relax/norelax is the best option. Having .option relax 'sticky' for symbol differences slightly reduces the amount of broken code if a user switches between relax/norelax in the same section, but you still have the potential for calls or branches being resolved incorrectly don't you?

[shiva0217]

We may need to find a way to indicate the .option norelax effective scope to handle the following case.

.option norelax
 call f1
.option relax
 call f2
.option norelax
 call f3

In this case, only call f2 should insert R_RISCV_RELAX relocation type.

One possible solution would be record the no relax flag to MCInst Flag and don't emit R_RISCV_RELAX if the instruction contains the flag.
Something like:

 class RISCVAsmParser : public MCTargetAsmParser {
+  bool NoRelaxLocStart = false;
   SMLoc getLoc() const { return getParser().getTok().getLoc(); }
 
@@ -1117,6 +1118,34 @@ bool RISCVAsmParser::parseDirectiveOption() {
     return false;
   }

+  if (Option == "relax") {
+    getTargetStreamer().emitDirectiveOptionRelax();
+
+    Parser.Lex();
+    if (Parser.getTok().isNot(AsmToken::EndOfStatement))
+      return Error(Parser.getTok().getLoc(),
+                   "unexpected token, expected end of statement");
+
+    setFeatureBits(RISCV::FeatureRelax, "relax");
+
+    // Update AsmBackend with new STI
+    getTargetStreamer().getStreamer().getAssemblerPtr()->getBackend()
+          .setSTI(getSTI());
+    NoRelaxLocStart = false;
+    return false;
+  }
+
+  if (Option == "norelax") {
+     getTargetStreamer().emitDirectiveOptionNoRelax();
+
+     Parser.Lex();
+     if (Parser.getTok().isNot(AsmToken::EndOfStatement))
+       return Error(Parser.getTok().getLoc(),
+                    "unexpected token, expected end of statement");
+    NoRelaxLocStart = true;
+    return false;
+  }
+
@@ -1244,6 +1273,9 @@ bool RISCVAsmParser::processInstruction(MCInst &Inst, SMLoc IDLoc,
                                       MCStreamer &Out) {
   Inst.setLoc(IDLoc);

+  if (NoRelaxLocStart)
+    Inst.setFlags(0x11);

+++ b/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
@@ -258,7 +258,7 @@ unsigned RISCVMCCodeEmitter::getImmOpValue(const MCInst &MI, unsigned OpNo,
       MCFixup::create(0, Expr, MCFixupKind(FixupKind), MI.getLoc()));
   ++MCNumFixups;

-  if (EnableRelax) {
+  if (EnableRelax && (MI.getFlags() != 0x11)) {

And only setting relax back to AsmBackend when parsing .option relax,
In this way, when the file contains .option relax/norelax interleaving,
the relaxation result could be correct and the instruction within .option norelax won't do relax due to lack of R_RISCV_RELAX.
Any thoughts?

[lewis-revill]

In D46423#1205666, @shiva0217 wrote:

We may need to find a way to indicate the .option norelax effective scope to handle the following case.

.option norelax
 call f1
.option relax
 call f2
.option norelax
 call f3

In this case, only call f2 should insert R_RISCV_RELAX relocation type.

One possible solution would be record the no relax flag to MCInst Flag and don't emit R_RISCV_RELAX if the instruction contains the flag.
Something like:

 class RISCVAsmParser : public MCTargetAsmParser {
+  bool NoRelaxLocStart = false;
   SMLoc getLoc() const { return getParser().getTok().getLoc(); }
 
@@ -1117,6 +1118,34 @@ bool RISCVAsmParser::parseDirectiveOption() {
     return false;
   }

+  if (Option == "relax") {
+    getTargetStreamer().emitDirectiveOptionRelax();
+
+    Parser.Lex();
+    if (Parser.getTok().isNot(AsmToken::EndOfStatement))
+      return Error(Parser.getTok().getLoc(),
+                   "unexpected token, expected end of statement");
+
+    setFeatureBits(RISCV::FeatureRelax, "relax");
+
+    // Update AsmBackend with new STI
+    getTargetStreamer().getStreamer().getAssemblerPtr()->getBackend()
+          .setSTI(getSTI());
+    NoRelaxLocStart = false;
+    return false;
+  }
+
+  if (Option == "norelax") {
+     getTargetStreamer().emitDirectiveOptionNoRelax();
+
+     Parser.Lex();
+     if (Parser.getTok().isNot(AsmToken::EndOfStatement))
+       return Error(Parser.getTok().getLoc(),
+                    "unexpected token, expected end of statement");
+    NoRelaxLocStart = true;
+    return false;
+  }
+
@@ -1244,6 +1273,9 @@ bool RISCVAsmParser::processInstruction(MCInst &Inst, SMLoc IDLoc,
                                       MCStreamer &Out) {
   Inst.setLoc(IDLoc);

+  if (NoRelaxLocStart)
+    Inst.setFlags(0x11);

+++ b/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
@@ -258,7 +258,7 @@ unsigned RISCVMCCodeEmitter::getImmOpValue(const MCInst &MI, unsigned OpNo,
       MCFixup::create(0, Expr, MCFixupKind(FixupKind), MI.getLoc()));
   ++MCNumFixups;

-  if (EnableRelax) {
+  if (EnableRelax && (MI.getFlags() != 0x11)) {

And only setting relax back to AsmBackend when parsing .option relax,
In this way, when the file contains .option relax/norelax interleaving,
the relaxation result could be correct and the instruction within .option norelax won't do relax due to lack of R_RISCV_RELAX.
Any thoughts?

I've been looking at this patch, and I can't quite see what difference adding the instruction flag makes? The following test works for me without the NoRelaxLocStart logic:

# RUN: llvm-mc -triple riscv32 < %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 riscv64 < %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

.option norelax
# CHECK-INST: call foo
# CHECK-RELOC: R_RISCV_CALL foo 0x0
# CHECK-RELOC-NOT: R_RISCV_RELAX foo 0x0
call foo

.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 norelax
# CHECK-INST: .option norelax
# CHECK-INST: call baz
# CHECK-RELOC-NEXT: R_RISCV_CALL baz 0x0
# CHECK-RELOC-NOT: R_RISCV_RELAX baz 0x0
call baz

Are you unable to replicate this?
Thanks

[shiva0217]

Apology. I thought the assembler will parse all the instructions and then do the encoding which is incorrect. It will parse and encode one instruction at a time. So we don't need using MCinst's Flag to record the instruction state and NoRelaxLocStart shouldn't need.
We could define FeatureNoRelaxREL feature to indicate the suppression of R_RISCV_RELAX relocation type instead of clear FeatureRelax bit.
Because once the .option relax occur in the file, shouldForceRelocation, and requiresDiffExpressionRelocations should return true to make sure the local branches and label differences will leave the relocation types.
With the relocation types, the linker relaxation could do the right adjustment when the code size changed.
I think it's quite similar to the approach @simoncook mention in https://reviews.llvm.org/D45181#1086541.
Is it a feasible way to implement? or we may still break something in certain cases?

if (Option == "relax") {
  getTargetStreamer().emitDirectiveOptionRelax();

  Parser.Lex();
  if (Parser.getTok().isNot(AsmToken::EndOfStatement))
    return Error(Parser.getTok().getLoc(),
                 "unexpected token, expected end of statement");

  setFeatureBits(RISCV::FeatureRelax, "relax");
 + clearFeatureBits(RISCV::FeatureNoRelaxREL, "norelaxrel");

  // Update AsmBackend with new STI
  getTargetStreamer().getStreamer().getAssemblerPtr()->getBackend()
      .setSTI(getSTI());
  return false;
}

if (Option == "norelax") {
  getTargetStreamer().emitDirectiveOptionNoRelax();

  Parser.Lex();
  if (Parser.getTok().isNot(AsmToken::EndOfStatement))
    return Error(Parser.getTok().getLoc(),
                 "unexpected token, expected end of statement");

  + setFeatureBits(RISCV::FeatureNoRelaxREL, "norelaxrel");
  // Update AsmBackend with new STI
  getTargetStreamer().getStreamer().getAssemblerPtr()->getBackend()
      .setSTI(getSTI());
  return false;
}

 +++ b/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
@@ -258,7 +258,7 @@ unsigned RISCVMCCodeEmitter::getImmOpValue(const MCInst &MI, unsigned OpNo,
       MCFixup::create(0, Expr, MCFixupKind(FixupKind), MI.getLoc()));
   ++MCNumFixups;

 -  if (EnableRelax) {
 +  bool NoRelaxREL = STI.getFeatureBits()[RISCV::FeatureNoRelaxREL];
 +  if (EnableRelax && (!NoRelaxREL)) {
       // Emit R_RISCV_RELAX relocation type.

[lewis-revill]

I've updated this revision to reflect the current state of what we're working with. The relax and norelax options work as intended with regards to enabling and disabling R_RISCV_RELAX. With this current patch, the behaviour of relocations is that if .option relax was ever parsed, then relocations are always generated. This is a safe approach which avoids incorrect codegen. Using the approach of only saving the STI state back to the backend when .option relax is parsed enables this behaviour. Other approaches might be a better fit, especially if any other future options need to save the state of the STI back to the backend, since this would invalidate the 'sticky bit' behaviour that we're looking for. I'll look into using the additional feature bit as you've suggested for this @shiva0217.

[lewis-revill]

Apologies for the successive updates, however I have updated the revision to utilize an additional feature bit to implement the 'sticky bit' behaviour due to the concerns I mentioned in the previous comment. This revision requires more extensive tests and comments, however it appears to be functional.

[shiva0217]

test/MC/RISCV/option-relax.s

11

Could you add tests that demonstrate the relocation types of branches and label differences will leave?

[lewis-revill]

Updated to address lack of comments and add relocation checks to the test.

[asb]

Many thanks for the update.

With the current revision, ForceLinkerReloc is defined but seems unused?

It might be worth spending more time looking at exactly what gas currently does. Looking at the build I've got handy, it seems it will currently unconditionally emit relocations for label differences (i.e. independent of whether -mno-relax is used). I suppose this further reduces cases where you might get incorrect codegen, but it does create a bunch of extra work for the linker which you'd hope -mno-relax would avoid.

It's going to be more hassle, but I think it would be worth evaluating passing through the current STI to the MCAsmBackend, like the series of patches culmininating in D45962.

[asb]

In fact, I'm now wondering if .option rvc/norvc fails to adjust the nop behaviour because MCAsmBackend doesn't get access to an updated STI.

[lewis-revill]

I've looked into the options for that change with regards to the behaviour that we'd want for this patch, and I think that while it is a good idea to make that change, it would not help us with our sticky bit situation. The problem is that adding an MCSubtargetInfo parameter to shouldForceRelocation in the same manner of the patches culminating in D45962 (IE: passing the STI that was used at the point of encoding the instruction) would effectively mean that if we were to check the 'sticky bit', it would be false in the case where a branch instruction occurs before an .option relax, even if the target location occurs after the .option relax, meaning incorrect code could be generated. The behaviour that we're looking for is a two-pass type behaviour, whereby if relaxation was ever enabled during parsing, then the only safe option is to emit a relocation.

With this in mind, I would like to look into our other options for fixing the interface issues with this patch, and I will also update the test case to reflect the behaviour that I just discussed as its not clear at the moment.

[lewis-revill]

Updated the interface used to update the state of the AsmBackend from the parser. This required moving the class definition into a separate header, but allows us to not pollute the MCAsmBackend interface. Also changed the method of tracking the force relocations 'sticky bit' to a field in AsmBackend, to remove unnecessary feature definitions.

Updated the test case to better reflect desired behaviour.

[lewis-revill]

@asb do you have any objections to the current interface? I can see that adding a header file for the AsmBackend could be disruptive but it allows us to keep the interface changes from affecting MCAsmBackend. The required change to the getAssemblerPtr method is curious, but I cannot find any rational reason for the method to return null given that getAssembler returns a value.

[asb]

Thanks for the updates Lewis. I think this is a good solution to the problem. I need to step through the code in more detail to give a final LGTM, but I think it resolves our questions.

Could you rename this to remove the WIP from the title?

The only potential issue is whether there is a user expectation that a file that starts with .option norelax should be assembled in the same way as passing norelax to the assembler. If relaxation is the default (which is the intention), then it won't. I'm just noting this for the record and in case anyone has a particular opinion on the matter, I don't think it's a blocking issue.

[lewis-revill]

@asb from the implementation I think it would cover the case where the -mrelax flag is passed but .option norelax is added to the beginning of the file, as the ForceRelocs field is never set by .option relax. Thinking about this though does reveal a possible flaw in this implementation in that if -mrelax is passed, then after an instruction that may be relaxed, .option norelax is enabled, diff relocations won't be emitted after the .option norelax, even though to be safe they should be. I'll look into updating the implementation to act correctly in both cases when I get the chance. I might need to split the testcsae into two files to make sure both cases are covered.

Thanks

[lewis-revill]

Just an update to my previous comment, I realised you were right @asb since we don't update the AsmBackend's SubtargetInfo anymore, so getFeatureBits()[RISCV::FeatureRelax] will still be true after parsing based on the -mattr=relax. I'm looking into potentially using only the ForceRelocs variable for relocation behaviour and trying to set this lazily when we parse an instruction that might be relaxed in a relaxable section to cover the case you mentioned. I'll update whether I think this is possible ASAP.

[asb]

In D46423#1243098, @lewis-revill wrote:

Just an update to my previous comment, I realised you were right @asb since we don't update the AsmBackend's SubtargetInfo anymore, so getFeatureBits()[RISCV::FeatureRelax] will still be true after parsing based on the -mattr=relax. I'm looking into potentially using only the ForceRelocs variable for relocation behaviour and trying to set this lazily when we parse an instruction that might be relaxed in a relaxable section to cover the case you mentioned. I'll update whether I think this is possible ASAP.

Thinking about it more, it's probably not worth worrying about the case where -mrelax is passed but .option norelax occurs early on in assembly. Or at least, it's a secondary concern to achieving the following (let me know if you think the desirable behaviour is something different):

• -mattr=+relax: Set FeatureRelax and ForceReloc. Even if .option norelax is encountered when parsing assembly, ForceReloc stays set.
• -mattr=-relax. FeatureRelax and ForceReloc are false. If .option relax is encountered then set FeatureRelax and ForceReloc. ForceReloc will not be unset, even if .option norelax is encountered.
• The above should happen both for the assembler and the 'direct codegen' codepath.

I think the direction you suggest with ForceRelocs should enable this, and probably makes the whole thing much easier to understand.

[lewis-revill]

So, I've updated this patch with what I believe is a good compromise. The AsmParser will check when it is parsing an instruction whether relaxation is enabled at that point, and if it is, the ForceRelocs field is set. In AsmBackend, ForceRelocs is now the only variable that affects whether relocations are emitted.

This allows the case where an early .option norelax is used to not emit unnecessary relocations. A test was also added for this case (option-norelax-override.s). Unfortunately the tests that currently test relocations would fail unless an instruction occured in the file to trigger ForceRelocs to be set, so some instructions had to be added to these files.

The compromise is that any instructions occuring during a relaxation section will cause ForceRelocs to be set, regardless of whether the instruction itself may be relaxed (explained in the comment in RISCVAsmParser.cpp).

[lewis-revill]

Just bumping this patch again, it would be nice to get this finalised/merged. I was planning to work on .option push/pop next, but it would require knowing the exact functionality of this.

[asb]

Could you add some tests that demonstrate this works as desired for a .ll file with inline assembly? e.g. if I'm compiling a .ll file with -mrelax and have inline assembly that includes no instructions but something like .dword .L2-.L1?

At that point, I think it's then good to go. Therefore it would be great if you could update the patch summary to describe the approach you ended up with.

Many thanks for your work on this.

[asb]

@kito-cheng, @shiva0217 - are you happy with this approach?

[lewis-revill]

Well apologies for the noise and the late update, but it looks like we can't have an early .option norelax overriding the behaviour specified with mattr=+relax. This is because the lazy ForceRelocs variable was only set when parsing assembly, so with a standard IR pipeline this is never set so relocations are never emitted. This update reverses this behaviour but keeps the lazy behaviour when parsing assembly when -mattr=-relax is used.

[asb]

In D46423#1255106, @lewis-revill wrote:

Well apologies for the noise and the late update, but it looks like we can't have an early .option norelax overriding the behaviour specified with mattr=+relax. This is because the lazy ForceRelocs variable was only set when parsing assembly, so with a standard IR pipeline this is never set so relocations are never emitted. This update reverses this behaviour but keeps the lazy behaviour when parsing assembly when -mattr=-relax is used.

I suspected that might be an issue. Can you double check the test coverage? I would have hoped to have seen some failures in test/CodeGen with the previous version of this patch. It looks like fixups-diff.ll is the only codegen test that would be affected, but I think it passed with the previous version of the patch when I tried it.

[lewis-revill]

Yes that is indeed the test that fails, I thought I had run the codegen tests with the updated patch but clearly not. Should I still look to add another IR test as you suggest or is fixup-diffs.ll sufficient for this?

[asb]

fixup-diffs.ll is sufficient to check this isn't breaking the desired behaviour for codegen. I think it would be great to update this patch with a .ll file with inline assembly that demonstrates the interaction there. We don't expect any problems based on how you've implemented this patch but it would be great to have the test there to capture the current behaviour in case we return to this logic in the future.

Update the patch description, and then I think we're good to commit.

[lewis-revill]

Added IR test to ensure diff relocation behaviour remains as expected through IR pipeline

[asb]

This looks good to me, thanks! Do you need me to commit?

[lewis-revill]

I would say so, I'm happy with it. Thank you.

[asb]

Shiva and Kito didn't seem to have any concerns so I was just re-checking prior to committing. I had a closer look at getAssemblerPtr and see this method was introduced by @niravd in rL331218. Nirav - are you happy for this patch to change getAssemblerPtr so it doesn't just return nullptr in MCStreamer.h? I don't think gating on UseAssemblerInfoForParsing makes sense in our case.

[niravd]

In D46423#1261724, @asb wrote:

Shiva and Kito didn't seem to have any concerns so I was just re-checking prior to committing. I had a closer look at getAssemblerPtr and see this method was introduced by @niravd in rL331218. Nirav - are you happy for this patch to change getAssemblerPtr so it doesn't just return nullptr in MCStreamer.h? I don't think gating on UseAssemblerInfoForParsing makes sense in our case.

No.

Disabling assembler-level information for parsing in the MCAsmStreamer was a compromise we reached for enforcing some consistency on assembler-dependent parsing to text and that relies on getAssemblerPtr() being nullptr, so it's not okay. (We should probably add a comment to that effect in MCAsmStreamer).

That said, it looks like the only reason this is coming up because you're using getAssemblerPtr() to access the backend. You should be able to leave getAssemblerPtr() as is and just use getAssembler() instead.

[lewis-revill]

Apologies for the delay, I was away for the week. The solution is to simply not bother updating the backend when the assembler pointer is null, since getAssemblerPtr returning nullptr implies that we are emitting textual assembly, not an object stream, so we don't need to worry about relaxation or relocation anyway.

[lewis-revill]

Rebased to apply cleanly on top of latest changes.

[jrtc27]

In D46423#1268188, @lewis-revill wrote:

Apologies for the delay, I was away for the week. The solution is to simply not bother updating the backend when the assembler pointer is null, since getAssemblerPtr returning nullptr implies that we are emitting textual assembly, not an object stream, so we don't need to worry about relaxation or relocation anyway.

I don't think that's true? The implementation of getAssemblerPtr is gated on getUseAssemblerInfoForParsing, which is only ever set to true by llvm-mc. Thus, if you have a -mno-relax C file that sets .option relax in inline assembly, the sticky flag won't be set, so things may break with -integrated-as. Now maybe we don't care about that case, but it's not so simple. Perhaps we could avoid this altogether by moving this flag to the RISCV subtarget instance (which will be copied just fine on push/pop as that's done via a clone-and-restore-feature-bits)?

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

1182

Missing newline.

[lewis-revill]

In D46423#1274042, @jrtc27 wrote:

In D46423#1268188, @lewis-revill wrote:

Apologies for the delay, I was away for the week. The solution is to simply not bother updating the backend when the assembler pointer is null, since getAssemblerPtr returning nullptr implies that we are emitting textual assembly, not an object stream, so we don't need to worry about relaxation or relocation anyway.

I don't think that's true? The implementation of getAssemblerPtr is gated on getUseAssemblerInfoForParsing, which is only ever set to true by llvm-mc. Thus, if you have a -mno-relax C file that sets .option relax in inline assembly, the sticky flag won't be set, so things may break with -integrated-as. Now maybe we don't care about that case, but it's not so simple. Perhaps we could avoid this altogether by moving this flag to the RISCV subtarget instance (which will be copied just fine on push/pop as that's done via a clone-and-restore-feature-bits)?

What happens is that when you go directly from C/IR straight to an object file the directives in inline assembly have no effect, since RISCVTargetELFStreamer::emitDirectiveOptionRelax is a no-op. All relax logic is done when actually parsing assembly files containing the directive.

This is consistent with the behaviour for .option rvc. I will add a test to make this explicit.

[lewis-revill]

Added explicit tests for ignoring inline assembly directive options during codegen when emitting directly to ELF.

[lewis-revill]

My explanation was incorrect; the empty emitDirectiveOptionRelax is not the reason the inline assembly does not effect relaxation. The reason is exactly the path you pointed out: changes the local feature bit state while parsing directive options from inline assembly will never be translated to the AsmBackend to be utilized in requiresDiffExpressionRelocations and shouldForceRelocation.

include/llvm/MC/MCAsmBackend.h

   ///
   /// \returns true iff any relaxation occurred.
   bool relaxFragment(MCPaddingFragment *PF, MCAsmLayout &Layout);
+
+  virtual void setSTI(const MCSubtargetInfo &STI) {};
 };
 
 } // end namespace llvm

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

 #include "MCTargetDesc/RISCVTargetStreamer.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/MC/MCAssembler.h"
+#include "llvm/MC/MCAsmBackend.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCExpr.h"
 #include "llvm/MC/MCInst.h"

     return false;
   }
 
+  if (Option == "relax") {
+    getTargetStreamer().emitDirectiveOptionRelax();
+
+    Parser.Lex();
+    if (Parser.getTok().isNot(AsmToken::EndOfStatement))
+      return Error(Parser.getTok().getLoc(),
+                   "unexpected token, expected end of statement");
+
+    setFeatureBits(RISCV::FeatureRelax, "relax");
+
+    // Update AsmBackend with new STI
+    getTargetStreamer().getStreamer().getAssemblerPtr()->getBackend()
+        .setSTI(getSTI());
+    return false;
+  }
+
+  if (Option == "norelax") {
+    getTargetStreamer().emitDirectiveOptionNoRelax();
+
+    Parser.Lex();
+    if (Parser.getTok().isNot(AsmToken::EndOfStatement))
+      return Error(Parser.getTok().getLoc(),
+                   "unexpected token, expected end of statement");
+
+    clearFeatureBits(RISCV::FeatureRelax, "relax");
+
+    // Update AsmBackend with new STI
+    getTargetStreamer().getStreamer().getAssemblerPtr()->getBackend()
+        .setSTI(getSTI());
+    return false;
+  }
+
   // Unknown option.
   Warning(Parser.getTok().getLoc(),
-          "unknown option, expected 'rvc' or 'norvc'");
+          "unknown option, expected 'rvc', 'norvc', 'relax' or 'norelax'");
   Parser.eatToEndOfStatement();
   return false;
 }

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

 
 namespace {
 class RISCVAsmBackend : public MCAsmBackend {
-  const MCSubtargetInfo &STI;
+  const MCSubtargetInfo *STI;
   uint8_t OSABI;
   bool Is64Bit;
 
 public:
   RISCVAsmBackend(const MCSubtargetInfo &STI, uint8_t OSABI, bool Is64Bit)
-      : MCAsmBackend(support::little), STI(STI), OSABI(OSABI),
+      : MCAsmBackend(support::little), STI(&STI), OSABI(OSABI),
         Is64Bit(Is64Bit) {}
   ~RISCVAsmBackend() override {}
 
   // Generate diff expression relocations if the relax feature is enabled,
   // otherwise it is safe for the assembler to calculate these internally.
   bool requiresDiffExpressionRelocations() const override {
-    return STI.getFeatureBits()[RISCV::FeatureRelax];
+    return STI->getFeatureBits()[RISCV::FeatureRelax];
   }
   void applyFixup(const MCAssembler &Asm, const MCFixup &Fixup,
                   const MCValue &Target, MutableArrayRef<char> Data,
                   uint64_t Value, bool IsResolved) const override;
 
+  void setSTI(const MCSubtargetInfo &_STI) override {STI = &_STI;}
+
   std::unique_ptr<MCObjectTargetWriter>
   createObjectTargetWriter() const override;
 

   // during relaxation.
   bool shouldForceRelocation(const MCAssembler &Asm, const MCFixup &Fixup,
                              const MCValue &Target) override {
-    return STI.getFeatureBits()[RISCV::FeatureRelax];
+    return STI->getFeatureBits()[RISCV::FeatureRelax];
   }
 
   bool fixupNeedsRelaxation(const MCFixup &Fixup, uint64_t Value,

 }
 
 bool RISCVAsmBackend::writeNopData(raw_ostream &OS, uint64_t Count) const {
-  bool HasStdExtC = STI.getFeatureBits()[RISCV::FeatureStdExtC];
+  bool HasStdExtC = STI->getFeatureBits()[RISCV::FeatureStdExtC];
   unsigned MinNopLen = HasStdExtC ? 2 : 4;
 
   if ((Count % MinNopLen) != 0)

lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.h

 
   virtual void emitDirectiveOptionRVC();
   virtual void emitDirectiveOptionNoRVC();
+  virtual void emitDirectiveOptionRelax();
+  virtual void emitDirectiveOptionNoRelax();
 };
 }
 #endif

lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

 
 void RISCVTargetELFStreamer::emitDirectiveOptionRVC() {}
 void RISCVTargetELFStreamer::emitDirectiveOptionNoRVC() {}
+void RISCVTargetELFStreamer::emitDirectiveOptionRelax() {}
+void RISCVTargetELFStreamer::emitDirectiveOptionNoRelax() {}

lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.h

 
   virtual void emitDirectiveOptionRVC() = 0;
   virtual void emitDirectiveOptionNoRVC() = 0;
+  virtual void emitDirectiveOptionRelax() = 0;
+  virtual void emitDirectiveOptionNoRelax() = 0;
 };
 
 // This part is for ascii assembly output

 
   void emitDirectiveOptionRVC() override;
   void emitDirectiveOptionNoRVC() override;
+  void emitDirectiveOptionRelax() override;
+  void emitDirectiveOptionNoRelax() override;
 };
 
 }

lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp

 void RISCVTargetAsmStreamer::emitDirectiveOptionNoRVC() {
   OS << "\t.option\tnorvc\n";
 }
+
+void RISCVTargetAsmStreamer::emitDirectiveOptionRelax() {
+  OS << "\t.option\trelax\n";
+}
+
+void RISCVTargetAsmStreamer::emitDirectiveOptionNoRelax() {
+  OS << "\t.option\tnorelax\n";
+}

test/MC/RISCV/option-invalid.s

 # CHECK: error: unexpected token, expected end of statement
 .option rvc foo
 
-# CHECK: warning: unknown option, expected 'rvc' or 'norvc'
+# CHECK: warning: unknown option, expected 'rvc', 'norvc', 'relax' or 'norelax'
 .option bar

test/MC/RISCV/option-relax.s

+# RUN: llvm-mc -triple riscv32 < %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 riscv64 < %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
+
+.L1:
+# CHECK-INST: call foo
+# CHECK-RELOC: R_RISCV_CALL foo 0x0
+call foo
+.quad .L2-.L1
+
+.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
+.quad .L2-.L1
+
+.L2:
+.option norelax
+# CHECK-INST: .option norelax
+# CHECK-INST: call baz
+# CHECK-RELOC: R_RISCV_CALL baz 0x0
+# CHECK-RELOC-NOT: R_RISCV_RELAX baz 0x0
+.quad .L2-.L1
+call baz