[AVR] Fix global references to function symbols

[amikhalev]

Summary

References to functions are in program memory and need a pm() fixup. This should fix trait objects for Rust on AVR.

[amikhalev]

Add more context to diff

[aykevl]

As a quick review (without looking into this carefully): you need to add tests. Without tests this will not be accepted. It also gives a better idea of what it does exactly and thus makes reviewing easier.

[amikhalev]

I added tests. One makes sure that a pm() asm expr generates a R_AVR_16_PM relocation. The other is pruned LLVM IR from a simple Rust program that uses trait objects.

[dylanmckay]

Good bugfix - thanks for the second patch!

llvm/lib/Target/AVR/AVRAsmPrinter.cpp

187

This check is not fully sufficient for covering all cases - namely, the constant may not be a function but still might be constrained to program memory and require the new pm fixup. Motivating example: a global variable that is explicitly located in program memory via an addrspace(1) qualifier.

Suggest replacing the "is a function" check with GV->getAddressSpace() == AVR::ProgramMemory which more directly captures what you're wanting to check, and will also add the new pm fixup to regular progmem vars

llvm/lib/Target/AVR/MCTargetDesc/AVRELFObjectWriter.cpp

11

Order the #include alphabetically

llvm/test/CodeGen/AVR/rust-trait-object.ll

10 ↗

(On Diff #292034)

Rename Rust identifier names in this file to something human readable. For this particular line, a more appropriate name might be EmptyType or simple TypeFoo. In general, LLVM tests are committed without the machine-readable naming conventions of the frontend used to generate them. It will also make some of the CHECK lines shorter and more obvious.

65 ↗

(On Diff #292034)

Remove attribute specifiers (#n) after functions - most if not all of them should be unnecessary

72 ↗

(On Diff #292034)

Drop the linking options and calling convention fastcc - I suspect these are not needed to reproduce

141 ↗

(On Diff #292034)

Delete the attributes which are not required for your test to pass

[dylanmckay]

llvm/test/CodeGen/AVR/rust-trait-object.ll

5 ↗

(On Diff #292034)

Optional: It might be a good idea to post the version of the Rust compiler used if it's not too much hassle to dig that up again.

67 ↗

(On Diff #292034)

Drop unnecessary function attributes as I suspect they are not required to reproduce the issue

92 ↗

(On Diff #292034)

Avoid numeric variable names in LLVM tests as they make modfications to the test in the future more difficult as LLVM verifier requires that all numerical vars are defined in order, which makes insertion of new variables mid-function problematic.

There is an existing LLVM pass which can be used to do this automatically, although there are very few numbered variable names in the test already so it might be easier to fixup manually. In the past I've been recommended and used to good success is the instnamer pass, invokable via opt -instnamer ..., although it will strip out your CHECK lines.

[dylanmckay]

I have applied the code review suggestions and committed this to master in 2ccb941740e608a9cf70a7c5840497149654b0f6.

This will fix https://github.com/rust-lang/rust/issues/79889

Thanks for the patch @amikhalev!

llvm/include/llvm/MC/MCExpr.h

     VK_AVR_DIFF8,
     VK_AVR_DIFF16,
     VK_AVR_DIFF32,
+    VK_AVR_PM,
 
     VK_PPC_LO,              // symbol@l
     VK_PPC_HI,              // symbol@h

llvm/lib/Target/AVR/AVRAsmPrinter.cpp

 #include "AVRMCInstLower.h"
 #include "AVRSubtarget.h"
 #include "MCTargetDesc/AVRInstPrinter.h"
+#include "MCTargetDesc/AVRMCExpr.h"
 #include "TargetInfo/AVRTargetInfo.h"
 
 #include "llvm/CodeGen/AsmPrinter.h"

 
   void emitInstruction(const MachineInstr *MI) override;
 
+  const MCExpr *lowerConstant(const Constant *CV) override;
+
 private:
   const MCRegisterInfo &MRI;
 };

   EmitToStreamer(*OutStreamer, I);
 }
 
+const MCExpr *AVRAsmPrinter::lowerConstant(const Constant *CV) {
+  MCContext &Ctx = OutContext;
+
+  if (const GlobalValue *GV = dyn_cast<GlobalValue>(CV)) {
+    bool IsFunction = isa<Function>(GV);
+    if (IsFunction) {
+      const MCExpr *Expr = MCSymbolRefExpr::create(getSymbol(GV), Ctx);
+      return AVRMCExpr::create(AVRMCExpr::VK_AVR_PM, Expr, false, Ctx);
+    }
+  }
+
+  return AsmPrinter::lowerConstant(CV);
+}
+
 } // end of namespace llvm
 
 extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeAVRAsmPrinter() {

llvm/lib/Target/AVR/MCTargetDesc/AVRELFObjectWriter.cpp

 
 #include "MCTargetDesc/AVRFixupKinds.h"
 #include "MCTargetDesc/AVRMCTargetDesc.h"
+#include "MCTargetDesc/AVRMCExpr.h"
 
 #include "llvm/MC/MCAssembler.h"
 #include "llvm/MC/MCELFObjectWriter.h"

     case MCSymbolRefExpr::VK_None:
       return ELF::R_AVR_16;
     case MCSymbolRefExpr::VK_AVR_NONE:
+    case MCSymbolRefExpr::VK_AVR_PM:
       return ELF::R_AVR_16_PM;
     case MCSymbolRefExpr::VK_AVR_DIFF16:
       return ELF::R_AVR_DIFF16;

llvm/lib/Target/AVR/MCTargetDesc/AVRMCExpr.h

 public:
   /// Specifies the type of an expression.
   enum VariantKind {
-    VK_AVR_None,
+    VK_AVR_None = 0,
 
     VK_AVR_HI8,  ///< Corresponds to `hi8()`.
     VK_AVR_LO8,  ///< Corresponds to `lo8()`.
     VK_AVR_HH8,  ///< Corresponds to `hlo8() and hh8()`.
     VK_AVR_HHI8, ///< Corresponds to `hhi8()`.
 
+    VK_AVR_PM,     ///< Corresponds to `pm()`, reference to program memory.
     VK_AVR_PM_LO8, ///< Corresponds to `pm_lo8()`.
     VK_AVR_PM_HI8, ///< Corresponds to `pm_hi8()`.
     VK_AVR_PM_HH8, ///< Corresponds to `pm_hh8()`.

llvm/lib/Target/AVR/MCTargetDesc/AVRMCExpr.cpp

     {"hh8", AVRMCExpr::VK_AVR_HH8}, // synonym with hlo8
     {"hlo8", AVRMCExpr::VK_AVR_HH8},      {"hhi8", AVRMCExpr::VK_AVR_HHI8},
 
+    {"pm", AVRMCExpr::VK_AVR_PM},
     {"pm_lo8", AVRMCExpr::VK_AVR_PM_LO8}, {"pm_hi8", AVRMCExpr::VK_AVR_PM_HI8},
     {"pm_hh8", AVRMCExpr::VK_AVR_PM_HH8},
 

     MCSymbolRefExpr::VariantKind Modifier = Sym->getKind();
     if (Modifier != MCSymbolRefExpr::VK_None)
       return false;
+    if (Kind == VK_AVR_PM) {
+      Modifier = MCSymbolRefExpr::VK_AVR_PM;
+    }
 
     Sym = MCSymbolRefExpr::create(&Sym->getSymbol(), Modifier, Context);
     Result = MCValue::get(Sym, Value.getSymB(), Value.getConstant());

     Value &= 0xff0000;
     Value >>= 16;
     break;
+  case AVRMCExpr::VK_AVR_PM:
   case AVRMCExpr::VK_AVR_GS:
     Value >>= 1; // Program memory addresses must always be shifted by one.
     break;

   case VK_AVR_PM_HH8:
     Kind = isNegated() ? AVR::fixup_hh8_ldi_pm_neg : AVR::fixup_hh8_ldi_pm;
     break;
+  case VK_AVR_PM:
   case VK_AVR_GS:
     Kind = AVR::fixup_16_pm;
     break;