[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

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/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 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::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/AVRMCTargetDesc.h"
+#include "MCTargetDesc/AVRMCExpr.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;