[PATCH] D46423: [WIP, RISCV] Support .option relax and .option norelax

[Lewis Revill via Phabricator via llvm-commits]

lewis-revill added a comment.

In https://reviews.llvm.org/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


Repository:
  rL LLVM

https://reviews.llvm.org/D46423