[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

12

Order the #include alphabetically

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

11

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.

66

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

73

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

142

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

[dylanmckay]

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

6

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.

68

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

93

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

enum VariantKind : uint16_t {
 
     VK_AVR_NONE,
     VK_AVR_LO8,
     VK_AVR_HI8,
     VK_AVR_HLO8,
     VK_AVR_DIFF8,
     VK_AVR_DIFF16,
     VK_AVR_DIFF32,
+    VK_AVR_PM,
 
     VK_PPC_LO,              // symbol@l
     VK_PPC_HI,              // symbol@h
     VK_PPC_HA,              // symbol@ha
     VK_PPC_HIGH,            // symbol@high
     VK_PPC_HIGHA,           // symbol@higha
     VK_PPC_HIGHER,          // symbol@higher
     VK_PPC_HIGHERA,         // symbol@highera

llvm/lib/MC/MCExpr.cpp

StringRef MCSymbolRefExpr::getVariantKindName(VariantKind Kind) {
   case VK_ARM_TLSDESCSEQ: return "tlsdescseq";
   case VK_AVR_NONE: return "none";
   case VK_AVR_LO8: return "lo8";
   case VK_AVR_HI8: return "hi8";
   case VK_AVR_HLO8: return "hlo8";
   case VK_AVR_DIFF8: return "diff8";
   case VK_AVR_DIFF16: return "diff16";
   case VK_AVR_DIFF32: return "diff32";
+  case VK_AVR_PM: return "pm";
   case VK_PPC_LO: return "l";
   case VK_PPC_HI: return "h";
   case VK_PPC_HA: return "ha";
   case VK_PPC_HIGH: return "high";
   case VK_PPC_HIGHA: return "higha";
   case VK_PPC_HIGHER: return "higher";
   case VK_PPC_HIGHERA: return "highera";
   case VK_PPC_HIGHEST: return "highest";

llvm/lib/Target/AVR/AVRAsmPrinter.cpp

 // of machine-dependent LLVM code to GAS-format AVR assembly language.
 //
 //===----------------------------------------------------------------------===//
 
 #include "AVR.h"
 #include "AVRMCInstLower.h"
 #include "AVRSubtarget.h"
 #include "MCTargetDesc/AVRInstPrinter.h"
+#include "MCTargetDesc/AVRMCExpr.h"
 #include "TargetInfo/AVRTargetInfo.h"
 
 #include "llvm/CodeGen/AsmPrinter.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineInstr.h"
 #include "llvm/CodeGen/TargetRegisterInfo.h"
 #include "llvm/CodeGen/TargetSubtargetInfo.h"
 #include "llvm/IR/Mangler.h"
public:
   bool PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
                        const char *ExtraCode, raw_ostream &O) override;
 
   bool PrintAsmMemoryOperand(const MachineInstr *MI, unsigned OpNum,
                              const char *ExtraCode, raw_ostream &O) override;
 
   void emitInstruction(const MachineInstr *MI) override;
 
+  const MCExpr *lowerConstant(const Constant *CV) override;
+
 private:
   const MCRegisterInfo &MRI;
 };
 
 void AVRAsmPrinter::printOperand(const MachineInstr *MI, unsigned OpNo,
                                  raw_ostream &O) {
   const MachineOperand &MO = MI->getOperand(OpNo);
 

 void AVRAsmPrinter::emitInstruction(const MachineInstr *MI) {
   AVRMCInstLower MCInstLowering(OutContext, *this);
 
   MCInst I;
   MCInstLowering.lowerInstruction(*MI, I);
   EmitToStreamer(*OutStreamer, I);
 }
 
+const MCExpr *AVRAsmPrinter::lowerConstant(const Constant *CV) {
+  MCContext &Ctx = OutContext;
+
+  if (const GlobalValue *GV = dyn_cast<GlobalValue>(CV)) {
+    bool IsProgMem = GV->getAddressSpace() == AVR::ProgramMemory;
+    if (IsProgMem) {
+      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::RegisterAsmPrinter<llvm::AVRAsmPrinter> X(llvm::getTheAVRTarget());
 }
 

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

 //===-- AVRELFObjectWriter.cpp - AVR ELF Writer ---------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "MCTargetDesc/AVRFixupKinds.h"
+#include "MCTargetDesc/AVRMCExpr.h"
 #include "MCTargetDesc/AVRMCTargetDesc.h"
 
 #include "llvm/MC/MCAssembler.h"
 #include "llvm/MC/MCELFObjectWriter.h"
 #include "llvm/MC/MCExpr.h"
 #include "llvm/MC/MCObjectWriter.h"
 #include "llvm/MC/MCSection.h"
 #include "llvm/MC/MCValue.h"
 #include "llvm/Support/ErrorHandling.h"
 
case FK_Data_4:
     }
   case FK_Data_2:
     switch (Modifier) {
     default:
       llvm_unreachable("Unsupported Modifier");
     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;
     }
   case AVR::fixup_32:
     return ELF::R_AVR_32;
   case AVR::fixup_7_pcrel:
     return ELF::R_AVR_7_PCREL;

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

 
 namespace llvm {
 
 /// A expression in AVR machine code.
 class AVRMCExpr : public MCTargetExpr {
 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()`.
 
     VK_AVR_LO8_GS, ///< Corresponds to `lo8(gs())`.
     VK_AVR_HI8_GS, ///< Corresponds to `hi8(gs())`.
     VK_AVR_GS, ///< Corresponds to `gs()`.
   };

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

 const struct ModifierEntry {
   const char * const Spelling;
   AVRMCExpr::VariantKind VariantKind;
 } ModifierNames[] = {
     {"lo8", AVRMCExpr::VK_AVR_LO8},       {"hi8", AVRMCExpr::VK_AVR_HI8},
     {"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},
 
     {"lo8_gs", AVRMCExpr::VK_AVR_LO8_GS}, {"hi8_gs", AVRMCExpr::VK_AVR_HI8_GS},
     {"gs", AVRMCExpr::VK_AVR_GS},
 };
 
 } // end of anonymous namespace
bool AVRMCExpr::evaluateAsRelocatableImpl(MCValue &Result,
   } else {
     if (!Layout) return false;
 
     MCContext &Context = Layout->getAssembler().getContext();
     const MCSymbolRefExpr *Sym = Value.getSymA();
     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());
   }
 
   return true;
 }
 
case AVRMCExpr::VK_AVR_HI8_GS:
     Value &= 0xff00;
     Value >>= 8;
     break;
   case AVRMCExpr::VK_AVR_PM_HH8:
     Value >>= 1; // Program memory addresses must always be shifted by one.
     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 AVRMCExpr::VK_AVR_None:
     llvm_unreachable("Uninitialized expression.");
   }
   return static_cast<uint64_t>(Value) & 0xff;
case VK_AVR_PM_LO8:
     Kind = isNegated() ? AVR::fixup_lo8_ldi_pm_neg : AVR::fixup_lo8_ldi_pm;
     break;
   case VK_AVR_PM_HI8:
     Kind = isNegated() ? AVR::fixup_hi8_ldi_pm_neg : AVR::fixup_hi8_ldi_pm;
     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;
   case VK_AVR_LO8_GS:
     Kind = AVR::fixup_lo8_ldi_gs;
     break;
   case VK_AVR_HI8_GS:
     Kind = AVR::fixup_hi8_ldi_gs;

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

+; RUN: llc < %s -march=avr -filetype=asm | FileCheck %s -check-prefix=CHECK-ASM
+; RUN: llc < %s -march=avr -filetype=obj | llvm-objdump -Dr - \
+; RUN:                                   | FileCheck %s -check-prefix=CHECK-OBJ
+
+; Somewhat pruned test case from rustc using trait objects
+
+%TraitObjectA = type {}
+
+; CHECK-ASM-LABEL: vtable.0:
+; CHECK-ASM-NEXT: .short pm(drop_in_place2)
+; CHECK-ASM-NEXT: .short 0
+; CHECK-ASM-NEXT: .short 1
+; CHECK-ASM-NEXT: .short pm(trait_fn2)
+
+; CHECK-OBJ-LABEL: <vtable.0>:
+; CHECK-OBJ-NEXT: 00 00
+; CHECK-OBJ-NEXT: R_AVR_16_PM  .text
+; CHECK-OBJ-NEXT: 00 00
+; CHECK-OBJ-NEXT: 01 00 00 00
+; CHECK-OBJ-NEXT: R_AVR_16_PM  .text
+@vtable.0 = private constant {
+    void (%TraitObjectA*) addrspace(1)*,
+    i16,
+    i16,
+    i8 (%TraitObjectA*) addrspace(1)*
+  } {
+    void (%TraitObjectA*) addrspace(1)*
+      @drop_in_place2,
+    i16 0,
+    i16 1,
+    i8 (%TraitObjectA*) addrspace(1)*
+      @trait_fn2
+  }, align 1
+
+; CHECK-ASM-LABEL: vtable.1:
+; CHECK-ASM-NEXT: .short pm(drop_in_place1)
+; CHECK-ASM-NEXT: .short 0
+; CHECK-ASM-NEXT: .short 1
+; CHECK-ASM-NEXT: .short pm(trait_fn1)
+
+; CHECK-OBJ-LABEL: <vtable.1>:
+; CHECK-OBJ-NEXT: 00 00
+; CHECK-OBJ-NEXT: R_AVR_16_PM  .text
+; CHECK-OBJ-NEXT: 00 00
+; CHECK-OBJ-NEXT: 01 00 00 00
+; CHECK-OBJ-NEXT: R_AVR_16_PM  .text
+@vtable.1 = private constant {
+    void (%TraitObjectA*) addrspace(1)*,
+    i16,
+    i16,
+    i8 (%TraitObjectA*) addrspace(1)*
+  } {
+    void (%TraitObjectA*) addrspace(1)*
+      @drop_in_place1,
+    i16 0,
+    i16 1,
+    i8 (%TraitObjectA*) addrspace(1)*
+      @trait_fn1
+  }, align 1
+
+define internal fastcc i8 @TraitObjectA_method(i1 zeroext %choice) addrspace(1) {
+start:
+  %b = alloca %TraitObjectA, align 1
+
+  %c = select i1 %choice, [3 x i16]* bitcast ({
+      void (%TraitObjectA*) addrspace(1)*,
+      i16,
+      i16,
+      i8 (%TraitObjectA*) addrspace(1)*
+    }* @vtable.0 to [3 x i16]*),
+    [3 x i16]* bitcast ({
+      void (%TraitObjectA*) addrspace(1)*,
+      i16,
+      i16,
+      i8 (%TraitObjectA*) addrspace(1)*
+    }* @vtable.1 to [3 x i16]*)
+  %b2 = bitcast %TraitObjectA* %b to {}*
+
+  %res = call fastcc addrspace(1) i8 @call_trait_object({}* nonnull align 1 %b2, [3 x i16]* noalias readonly align 1 dereferenceable(6) %c)
+  ret i8 %res
+}
+
+define internal fastcc i8 @call_trait_object({}* nonnull align 1 %a, [3 x i16]* noalias nocapture readonly align 1 dereferenceable(6) %b) addrspace(1) {
+start:
+  %b2 = getelementptr inbounds [3 x i16], [3 x i16]* %b, i16 0, i16 3
+  %c = bitcast i16* %b2 to i8 ({}*) addrspace(1)**
+  %d = load i8 ({}*) addrspace(1)*, i8 ({}*) addrspace(1)** %c, align 1, !invariant.load !1, !nonnull !1
+  %res = tail call addrspace(1) i8 %d({}* nonnull align 1 %a)
+  ret i8 %res
+}
+
+define internal void @drop_in_place1(%TraitObjectA* nocapture %a) addrspace(1) {
+start:
+  ret void
+}
+
+define internal i8 @trait_fn1(%TraitObjectA* noalias nocapture nonnull readonly align 1 %self) addrspace(1) {
+start:
+  ret i8 89
+}
+
+define internal void @drop_in_place2(%TraitObjectA* nocapture %a) addrspace(1) {
+start:
+  ret void
+}
+
+define internal i8 @trait_fn2(%TraitObjectA* noalias nocapture nonnull readonly align 1 %self) addrspace(1) {
+start:
+  ret i8 79
+}
+
+!1 = !{}

llvm/test/MC/AVR/relocations.s

 ldi r25, pm_hh8(-(foo))
 
 ; CHECK-NEXT: R_AVR_LO8_LDI_GS foo
 ldi r17, lo8(gs(foo))
 
 ; CHECK-NEXT: R_AVR_HI8_LDI_GS foo
 ldi r18, hi8(gs(foo))
 
-; CHECK-NEXT: R_AVR_16
+; CHECK-NEXT: R_AVR_16 foo
 .short foo
 
-; CHECK-NEXT: R_AVR_16_PM
+; CHECK-NEXT: R_AVR_16_PM foo
 .short gs(foo)
 
+; CHECK-NEXT: R_AVR_16_PM foo
+.short pm(foo)
+
 ; CHECK-NEXT: R_AVR_8
 .byte foo
 
 ; CHECK-NEXT: R_AVR_8_LO8
 .byte lo8(foo)
 
 ; CHECK-NEXT: R_AVR_8_HI8
 .byte hi8(foo)