[RISCV][MC] Fixed error: could not find corresponding %pcrel_hi

[apazos]

Summary

When parsing instructions, we need to provide the subtarget to detect when the subtarget changes due to options as they are parsed.
This affects the fragment the label will be placed in.

[apazos]

Lewis, is this what you had in mind, I only changed RISC-V parser path.

[lewis-revill]

In D58759#1413207, @apazos wrote:

Lewis, is this what you had in mind, I only changed RISC-V parser path.

Yes, something very similar, thank you. I just added an additional EmitLabel, with a default implementation which just called the original EmitLabel, and override it for MCELFStreamer. If this patch compiles for all targets though I don't see any problem with it.

[apazos]

Hi Lewis, yes it is possible to reduce the code changes. I will push an update.
The concern I have is that for pseudo instructions it was enough to change RISCVAsmParser.cpp to fix the problem. But for the expanded instructions I have to modify AsmParser.cpp.
So maybe instead of adding a new EmitLabel we change the current api.

[apazos]

Reduced code changes, added test case

[apazos]

Updated triple in test case

[efriedma]

I don't understand why it necessary to ensure the label is attached to the "correct" fragment; it's still pointing to the same address either way. Could you try a little harder in RISCVMCExpr::getPCRelHiFixup() to find the symbol? (Given an MCFragment, you can call getPrevNode() to get the previous MCFragment.)

[lewis-revill]

In D58759#1415688, @efriedma wrote:

I don't understand why it necessary to ensure the label is attached to the "correct" fragment; it's still pointing to the same address either way. Could you try a little harder in RISCVMCExpr::getPCRelHiFixup() to find the symbol? (Given an MCFragment, you can call getPrevNode() to get the previous MCFragment.)

I guess the difficulty there is that you wouldn't know which fragment the symbol offset (AUIPCSymbol->getOffset()) relates to. So you might end up finding a symbol with a matching offset in the current fragment even though you should be looking in the previous fragment, or vice-versa.

[lewis-revill]

Could you re-title the revision to start with [RISCV] and preferably [MC]? As it is, it may not be seen by all the relevant people.

[apazos]

Hi Eli,

The pair of instructions constraint is that the instruction (or instructions) with the LO12 relocation label points to a valid HI20 relocation which in turn points to the symbol:

At label: R_RISCV_PCREL_HI20 relocation entry ⟶ symbol
R_RISCV_PCREL_LO12_I relocation entry ⟶ label

See https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md#relocations

Let us know if you have a suggestion on how to do this matching some other way.

[efriedma]

I guess the difficulty there is that you wouldn't know which fragment the symbol offset (AUIPCSymbol->getOffset()) relates to.

Either the label is in the same fragment as the fixup, so the current code works, or the label points to the end of a fragment and the fixup is at offset zero in the next fragment. Otherwise, they can't match because there's data in between the label and the fixup. (I guess it's a little more complicated if you consider the possibility of zero-size fragments, but that doesn't make it a lot more complicated.)

I'm concerned the approach in current patch can't be generalized to handle all cases, for example:

.option push
.option norelax
nop
2:
.option pop
auipc   a1, %pcrel_hi(another_symbol)
addi    a1, a1, %pcrel_lo(2b)

include/llvm/MC/MCStreamer.h

443

If we're going to do this, we should give this a better name... maybe something like "EmitLabelInNextFragment" or something like that, to indicate that it's actually doing something unusual.

[efriedma]

Posted https://reviews.llvm.org/D58943, which implements my proposal. Hopefully it makes sense.

[apazos]

Hi Eli, this current patch as it is fails for the test case you highlighted. Thanks for clarifying and pushing an alternative solution.

[apazos]

We are going with the alternative in https://reviews.llvm.org/D58943

include/llvm/MC/MCELFStreamer.h

public:
   }
 
   /// \name MCStreamer Interface
   /// @{
 
   void InitSections(bool NoExecStack) override;
   void ChangeSection(MCSection *Section, const MCExpr *Subsection) override;
   void EmitLabel(MCSymbol *Symbol, SMLoc Loc = SMLoc()) override;
+  void EmitLabelEx(MCSymbol *Symbol, SMLoc Loc = SMLoc(),
+                   const MCSubtargetInfo *STI = nullptr) override;
   void EmitLabel(MCSymbol *Symbol, SMLoc Loc, MCFragment *F) override;
   void EmitAssemblerFlag(MCAssemblerFlag Flag) override;
   void EmitThumbFunc(MCSymbol *Func) override;
   void EmitWeakReference(MCSymbol *Alias, const MCSymbol *Symbol) override;
   bool EmitSymbolAttribute(MCSymbol *Symbol, MCSymbolAttr Attribute) override;
   void EmitSymbolDesc(MCSymbol *Symbol, unsigned DescValue) override;
   void EmitCommonSymbol(MCSymbol *Symbol, uint64_t Size,
                         unsigned ByteAlignment) override;

include/llvm/MC/MCObjectStreamer.h

public:
   void flushPendingLabels() { flushPendingLabels(nullptr); }
 
   MCAssembler &getAssembler() { return *Assembler; }
   MCAssembler *getAssemblerPtr() override;
   /// \name MCStreamer Interface
   /// @{
 
   void EmitLabel(MCSymbol *Symbol, SMLoc Loc = SMLoc()) override;
+  void EmitLabelEx(MCSymbol *Symbol, SMLoc Loc = SMLoc(),
+                   const MCSubtargetInfo *STI = nullptr) override;
   virtual void EmitLabel(MCSymbol *Symbol, SMLoc Loc, MCFragment *F);
   void EmitAssignment(MCSymbol *Symbol, const MCExpr *Value) override;
   void EmitValueImpl(const MCExpr *Value, unsigned Size,
                      SMLoc Loc = SMLoc()) override;
   void EmitULEB128Value(const MCExpr *Value) override;
   void EmitSLEB128Value(const MCExpr *Value) override;
   void EmitWeakReference(MCSymbol *Alias, const MCSymbol *Symbol) override;
   void ChangeSection(MCSection *Section, const MCExpr *Subsection) override;

include/llvm/MC/MCStreamer.h

public:
   ///
   /// \param Symbol - The symbol to emit. A given symbol should only be
   /// emitted as a label once, and symbols emitted as a label should never be
   /// used in an assignment.
   // FIXME: These emission are non-const because we mutate the symbol to
   // add the section we're emitting it to later.
   virtual void EmitLabel(MCSymbol *Symbol, SMLoc Loc = SMLoc());
 
+  /// Similar to EmitLabel but uses subtarget info to check if symbol
+  /// can be emitted in the current fragment.
+  virtual void EmitLabelEx(MCSymbol *Symbol, SMLoc Loc = SMLoc(),
+                           const MCSubtargetInfo *STI = nullptr);
+
   virtual void EmitEHSymAttributes(const MCSymbol *Symbol, MCSymbol *EHSymbol);
 
   /// Note in the output the specified \p Flag.
   virtual void EmitAssemblerFlag(MCAssemblerFlag Flag);
 
   /// Emit the given list \p Options of strings as linker
   /// options into the output.
   virtual void EmitLinkerOptions(ArrayRef<std::string> Kind) {}

lib/MC/MCELFStreamer.cpp

void MCELFStreamer::InitSections(bool NoExecStack) {
   SwitchSection(Ctx.getObjectFileInfo()->getTextSection());
   EmitCodeAlignment(4);
 
   if (NoExecStack)
     SwitchSection(Ctx.getAsmInfo()->getNonexecutableStackSection(Ctx));
 }
 
 void MCELFStreamer::EmitLabel(MCSymbol *S, SMLoc Loc) {
+  return EmitLabelEx(S, Loc);
+}
+
+void MCELFStreamer::EmitLabelEx(MCSymbol *S, SMLoc Loc,
+                                const MCSubtargetInfo *STI) {
   auto *Symbol = cast<MCSymbolELF>(S);
-  MCObjectStreamer::EmitLabel(Symbol, Loc);
+  MCObjectStreamer::EmitLabelEx(Symbol, Loc, STI);
 
   const MCSectionELF &Section =
       static_cast<const MCSectionELF &>(*getCurrentSectionOnly());
   if (Section.getFlags() & ELF::SHF_TLS)
     Symbol->setType(ELF::STT_TLS);
 }
 
 void MCELFStreamer::EmitLabel(MCSymbol *S, SMLoc Loc, MCFragment *F) {

lib/MC/MCObjectStreamer.cpp

 }
 
 void MCObjectStreamer::EmitCFIEndProcImpl(MCDwarfFrameInfo &Frame) {
   Frame.End = getContext().createTempSymbol();
   EmitLabel(Frame.End);
 }
 
 void MCObjectStreamer::EmitLabel(MCSymbol *Symbol, SMLoc Loc) {
+  return EmitLabelEx(Symbol, Loc);
+}
+
+void MCObjectStreamer::EmitLabelEx(MCSymbol *Symbol, SMLoc Loc,
+                                  const MCSubtargetInfo *STI) {
   MCStreamer::EmitLabel(Symbol, Loc);
 
   getAssembler().registerSymbol(*Symbol);
 
   // If there is a current fragment, mark the symbol as pointing into it.
   // Otherwise queue the label and set its fragment pointer when we emit the
   // next fragment.
   auto *F = dyn_cast_or_null<MCDataFragment>(getCurrentFragment());
-  if (F && !(getAssembler().isBundlingEnabled() &&
-             getAssembler().getRelaxAll())) {
+  if (F && CanReuseDataFragment(*F, getAssembler(), STI)) {
     Symbol->setFragment(F);
     Symbol->setOffset(F->getContents().size());
   } else {
     PendingLabels.push_back(Symbol);
   }
 }
 
 void MCObjectStreamer::EmitLabel(MCSymbol *Symbol, SMLoc Loc, MCFragment *F) {

lib/MC/MCParser/AsmParser.cpp

case AsmToken::Colon: {
     if (getTok().is(AsmToken::EndOfStatement)) {
       Lex();
     }
 
     getTargetParser().doBeforeLabelEmit(Sym);
 
     // Emit the label.
     if (!getTargetParser().isParsingInlineAsm())
-      Out.EmitLabel(Sym, IDLoc);
+      Out.EmitLabelEx(Sym, IDLoc, &getTargetParser().getSTI());
 
     // If we are generating dwarf for assembly source files then gather the
     // info to make a dwarf label entry for this label if needed.
     if (enabledGenDwarfForAssembly())
       MCGenDwarfLabelEntry::Make(Sym, &getStreamer(), getSourceManager(),
                                  IDLoc);
 
     getTargetParser().onLabelParsed(Sym);

lib/MC/MCStreamer.cpp

void MCStreamer::AssignFragment(MCSymbol *Symbol, MCFragment *Fragment) {
   assert(Fragment);
   Symbol->setFragment(Fragment);
 
   // As we emit symbols into a section, track the order so that they can
   // be sorted upon later. Zero is reserved to mean 'unemitted'.
   SymbolOrdering[Symbol] = 1 + SymbolOrdering.size();
 }
 
+void MCStreamer::EmitLabelEx(MCSymbol *Symbol, SMLoc Loc,
+                             const MCSubtargetInfo *STI) {
+  // Default implementation does not use STI.
+  return EmitLabel(Symbol, Loc);
+}
 void MCStreamer::EmitLabel(MCSymbol *Symbol, SMLoc Loc) {
   Symbol->redefineIfPossible();
 
   if (!Symbol->isUndefined() || Symbol->isVariable())
     return getContext().reportError(Loc, "invalid symbol redefinition");
 
   assert(!Symbol->isVariable() && "Cannot emit a variable symbol!");
   assert(getCurrentSectionOnly() && "Cannot emit before setting section!");

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

void RISCVAsmParser::emitAuipcInstPair(MCOperand DestReg, MCOperand TmpReg,
                                        MCStreamer &Out) {
   // A pair of instructions for PC-relative addressing; expands to
   //   TmpLabel: AUIPC TmpReg, VKHi(symbol)
   //             OP DestReg, TmpReg, %pcrel_lo(TmpLabel)
   MCContext &Ctx = getContext();
 
   MCSymbol *TmpLabel = Ctx.createTempSymbol(
       "pcrel_hi", /* AlwaysAddSuffix */ true, /* CanBeUnnamed */ false);
-  Out.EmitLabel(TmpLabel);
+  Out.EmitLabelEx(TmpLabel, IDLoc, &getSTI());
 
   const RISCVMCExpr *SymbolHi = RISCVMCExpr::create(Symbol, VKHi, Ctx);
   emitToStreamer(
       Out, MCInstBuilder(RISCV::AUIPC).addOperand(TmpReg).addExpr(SymbolHi));
 
   const MCExpr *RefToLinkTmpLabel =
       RISCVMCExpr::create(MCSymbolRefExpr::create(TmpLabel, Ctx),
                           RISCVMCExpr::VK_RISCV_PCREL_LO, Ctx);

test/MC/RISCV/option-mix.s

+# RUN: llvm-mc %s -triple=riscv32 | FileCheck -check-prefix=ASM %s
+# RUN: llvm-mc %s -triple=riscv64 | FileCheck -check-prefix=ASM %s
+# RUN: llvm-mc -filetype=obj -triple riscv32 < %s \
+# RUN:   | llvm-objdump -d -riscv-no-aliases - | FileCheck -check-prefix=DISASM %s
+# RUN: llvm-mc -filetype=obj -triple riscv64 < %s \
+# RUN:   | llvm-objdump -d -riscv-no-aliases - | FileCheck -check-prefix=DISASM %s
+
+# Checks change of options does not cause error: could not find corresponding %pcrel_hi 
+# when assembling pseudoinstruction and its extended form.
+
+.option push
+.option norelax
+  la a0, a_symbol
+.option pop
+  la a1, another_symbol
+
+# ASM: .Lpcrel_hi0:
+# ASM: auipc   a0, %pcrel_hi(a_symbol)
+# ASM: addi    a0, a0, %pcrel_lo(.Lpcrel_hi0)
+# ASM: .Lpcrel_hi1:
+# ASM: auipc   a1, %pcrel_hi(another_symbol)
+# ASM: addi    a1, a1, %pcrel_lo(.Lpcrel_hi1)
+
+# DISASM: .Lpcrel_hi0:
+# DISASM: auipc   a0, 0
+# DISASM: addi    a0, a0, 0
+# DISASM:.Lpcrel_hi1:
+# DISASM: auipc   a1, 0
+# DISASM: addi    a1, a1, 0
+
+.option push
+.option norelax
+1:auipc   a0, %pcrel_hi(a_symbol)
+  addi    a0, a0, %pcrel_lo(1b)
+.option pop
+2:auipc   a1, %pcrel_hi(another_symbol)
+  addi    a1, a1, %pcrel_lo(2b)
+
+# ASM: .Ltmp0:
+# ASM: auipc   a0, %pcrel_hi(a_symbol)
+# ASM: addi    a0, a0, %pcrel_lo(.Ltmp0)
+# ASM: .Ltmp1:
+# ASM: auipc   a1, %pcrel_hi(another_symbol)
+# ASM: addi    a1, a1, %pcrel_lo(.Ltmp1)
+
+# DISASM: .Ltmp0:
+# DISASM: auipc   a0, 0
+# DISASM: addi    a0, a0, 0
+# DISASM: .Ltmp1:
+# DISASM: auipc   a1, 0
+# DISASM: addi    a1, a1, 0