[RFC][RISCV] Don't disassemble `addi`s with relocations as `mv`s

[luismarques]

Summary

A RISC-V mv rd, rs instruction is an alias/pseudoinstruction for addi rd, rs, 0.

When disassembling object files we'll commonly encounter many (arguably) spurious mvs: addis with unresolved relocations. The immediate operand isn't actually zero, though that is obscured by the relocation, so IMO it shouldn't be disassembled as a mv. I'd argue that even if such a relocation happens to resolve to zero it's still semantically not a mv, but that is a minor point.

This patch tries to address that situation by detecting when those addis have relocations and not printing them as mvs.

Please let me know if you agree with (1) my reasoning and (2) the overall approach of this patch.
If there is a rough agreement I'll further improve the patch, including optimizing the search for the relocations and not introducing a separate printInst method.

[craig.topper]

llvm/tools/llvm-objdump/llvm-objdump.cpp

535

Do we have an llvm::find_if that uses ranges?

[craig.topper]

I think the reasoning makes sense to me. Do you also use -r when you disassemble object files? Or do you anticipate just knowing that an addi with 0 is some unrelocated address?

[reames]

Basic motivation/direction here makes sense to me. +1 on the general approach.

[luismarques]

In D143345#4129968, @craig.topper wrote:

I think the reasoning makes sense to me. Do you also use -r when you disassemble object files? Or do you anticipate just knowing that an addi with 0 is some unrelocated address?

I often use the -r option to determine the symbol addresses being computed (in non-trivial disassemblies) or as a sanity check to verify that some immediate isn't actually zero. Sometimes I don't bother because it makes the output noisy or I just forget.

I'm not sure I understand your second question, though (it's late here...). I assume it's more than just a curiosity and it has some relation to this patch but it's not clear to me what the implication is. Yes, without -r the disassemblies can be a bit puzzling at times, and you need to anticipate that some 0 immediates are misleading, and they won't actually be zero in the final binary. You have the same problem with mvs, just exacerbated because then you have to think that they aren't even true mv instructions at all. Is your point that when you do use -r it should be clear that the mvs aren't actually moves and the addis aren't actually zero? Apologies if I'm being dense.

[luismarques]

In D143345#4130539, @luismarques wrote:

I'm not sure I understand your second question, though (it's late here...). I assume it's more than just a curiosity and it has some relation to this patch but it's not clear to me what the implication is. Yes, without -r the disassemblies can be a bit puzzling at times, and you need to anticipate that some 0 immediates are misleading, and they won't actually be zero in the final binary. You have the same problem with mvs, just exacerbated because then you have to think that they aren't even true mv instructions at all. Is your point that when you do use -r it should be clear that the mvs aren't actually moves and the addis aren't actually zero? Apologies if I'm being dense.

I assume it's the latter. It's true that with -r the spurious mvs are less of a problem, though I think it's still something to be avoided if we can.

[craig.topper]

In D143345#4130565, @luismarques wrote:

In D143345#4130539, @luismarques wrote:

I'm not sure I understand your second question, though (it's late here...). I assume it's more than just a curiosity and it has some relation to this patch but it's not clear to me what the implication is. Yes, without -r the disassemblies can be a bit puzzling at times, and you need to anticipate that some 0 immediates are misleading, and they won't actually be zero in the final binary. You have the same problem with mvs, just exacerbated because then you have to think that they aren't even true mv instructions at all. Is your point that when you do use -r it should be clear that the mvs aren't actually moves and the addis aren't actually zero? Apologies if I'm being dense.

I assume it's the latter. It's true that with -r the spurious mvs are less of a problem, though I think it's still something to be avoided if we can.

Yes it was the latter. I was just curious.

It'd be really cool if we could use the relocation to print %pcrel_lo(sym) instead of 0. But I don't know how much work that would be.

[luismarques]

In D143345#4130585, @craig.topper wrote:

It'd be really cool if we could use the relocation to print %pcrel_lo(sym) instead of 0. But I don't know how much work that would be.

I can work on that if there is agreement that we should go in that direction. Makes sense to me, but I wonder if some people will push back against that, regardless of implementation concerns.

[jrtc27]

In D143345#4130588, @luismarques wrote:

In D143345#4130585, @craig.topper wrote:

It'd be really cool if we could use the relocation to print %pcrel_lo(sym) instead of 0. But I don't know how much work that would be.

I can work on that if there is agreement that we should go in that direction. Makes sense to me, but I wonder if some people will push back against that, regardless of implementation concerns.

From my perspective that would be cool and useful for all targets. I can’t think of a reason why one wouldn’t prefer that, beyond the complexity of having to synthesise labels when the assembler has converted them to section-relative.

llvm/include/llvm/MC/MCInstPrinter.h

public:
   /// \p Address the address of current instruction on most targets, used to
   /// print a PC relative immediate as the target address. On targets where a PC
   /// relative immediate is relative to the next instruction and the length of a
   /// MCInst is difficult to measure (e.g. x86), this is the address of the next
   /// instruction. If Address is 0, the immediate will be printed.
   virtual void printInst(const MCInst *MI, uint64_t Address, StringRef Annot,
                          const MCSubtargetInfo &STI, raw_ostream &OS) = 0;
 
+  virtual void printInst(const MCInst *MI, uint64_t Address, StringRef Annot,
+                         const MCSubtargetInfo &STI, raw_ostream &OS,
+                         bool HasRel) {
+    printInst(MI, Address, Annot, STI, OS);
+  }
+
   /// Return the name of the specified opcode enum (e.g. "MOV32ri") or
   /// empty if we can't resolve it.
   StringRef getOpcodeName(unsigned Opcode) const;
 
   /// Print the assembler register name.
   virtual void printRegName(raw_ostream &OS, MCRegister Reg) const;
 
   bool getUseMarkup() const { return UseMarkup; }

llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.h

public:
   RISCVInstPrinter(const MCAsmInfo &MAI, const MCInstrInfo &MII,
                    const MCRegisterInfo &MRI)
       : MCInstPrinter(MAI, MII, MRI) {}
 
   bool applyTargetSpecificCLOption(StringRef Opt) override;
 
   void printInst(const MCInst *MI, uint64_t Address, StringRef Annot,
                  const MCSubtargetInfo &STI, raw_ostream &O) override;
+  void printInst(const MCInst *MI, uint64_t Address, StringRef Annot,
+                 const MCSubtargetInfo &STI, raw_ostream &O,
+                 bool HasRel) override;
   void printRegName(raw_ostream &O, MCRegister Reg) const override;
 
   void printOperand(const MCInst *MI, unsigned OpNo, const MCSubtargetInfo &STI,
                     raw_ostream &O, const char *Modifier = nullptr);
   void printBranchOperand(const MCInst *MI, uint64_t Address, unsigned OpNo,
                           const MCSubtargetInfo &STI, raw_ostream &O);
   void printCSRSystemRegister(const MCInst *MI, unsigned OpNo,
                               const MCSubtargetInfo &STI, raw_ostream &O);

llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp

if (Opt == "numeric") {
     return true;
   }
 
   return false;
 }
 
 void RISCVInstPrinter::printInst(const MCInst *MI, uint64_t Address,
                                  StringRef Annot, const MCSubtargetInfo &STI,
-                                 raw_ostream &O) {
+                                 raw_ostream &O, bool HasRel) {
   bool Res = false;
   const MCInst *NewMI = MI;
   MCInst UncompressedMI;
   if (PrintAliases && !NoAliases)
     Res = RISCVRVC::uncompress(UncompressedMI, *MI, STI);
   if (Res)
     NewMI = const_cast<MCInst *>(&UncompressedMI);
+  if (MI->getOpcode() == RISCV::ADDI) {
+    auto Rs = MI->getOperand(1);
+    if (Rs.getReg() != RISCV::X0) { // neither a "nop" nor a "li".
+      auto Imm = MI->getOperand(2);
+      if (Imm.isImm() && Imm.getImm() == 0 && HasRel)
+        NoAliases = true; // don't print the "addi" of a "la" as "mv".
+    }
+  }
   if (!PrintAliases || NoAliases || !printAliasInstr(NewMI, Address, STI, O))
     printInstruction(NewMI, Address, STI, O);
   printAnnotation(O, Annot);
 }
 
+void RISCVInstPrinter::printInst(const MCInst *MI, uint64_t Address,
+                                 StringRef Annot, const MCSubtargetInfo &STI,
+                                 raw_ostream &O) {
+  printInst(MI, Address, Annot, STI, O, false);
+}
+
 void RISCVInstPrinter::printRegName(raw_ostream &O, MCRegister Reg) const {
   O << getRegisterName(Reg);
 }
 
 void RISCVInstPrinter::printOperand(const MCInst *MI, unsigned OpNo,
                                     const MCSubtargetInfo &STI, raw_ostream &O,
                                     const char *Modifier) {
   assert((Modifier == nullptr || Modifier[0] == 0) && "No modifiers supported");

llvm/test/MC/RISCV/rvi-aliases-valid.s

 
 # CHECK-S-OBJ-NOALIAS: ebreak
 # CHECK-S-OBJ: ebreak
 sbreak
 
 # CHECK-S-OBJ-NOALIAS: ecall
 # CHECK-S-OBJ: ecall
 scall
+
+.option relax
+1: auipc t1, %pcrel_hi(1b)
+# CHECK-OBJ-NOALIAS: addi t1, t1, 0
+# CHECK-OBJ: addi t1, t1, 0
+   addi  t1, t1, %pcrel_lo(1b)

llvm/tools/llvm-objdump/llvm-objdump.cpp

printInst(MCInstPrinter &IP, const MCInst *MI, ArrayRef<uint8_t> Bytes,
 
     if (MI) {
       // See MCInstPrinter::printInst. On targets where a PC relative immediate
       // is relative to the next instruction and the length of a MCInst is
       // difficult to measure (x86), this is the address of the next
       // instruction.
       uint64_t Addr =
           Address.Address + (STI.getTargetTriple().isX86() ? Bytes.size() : 0);
-      IP.printInst(MI, Addr, "", STI, OS);
+      auto is_rel = [=](RelocationRef Rel) { return Rel.getOffset() == Addr; };
+      // TODO: if people agree with the overall approach of this patch then the
+      // search for a relocation will be optimized to avoid duplicate work.
+      bool HasRel =
+          std::find_if(Rels->begin(), Rels->end(), is_rel) != Rels->end();
+      IP.printInst(MI, Addr, "", STI, OS, HasRel);
     } else
       OS << "\t<unknown>";
   }
 };
 PrettyPrinter PrettyPrinterInst;
 
 class HexagonPrettyPrinter : public PrettyPrinter {
 public: