[PATCH] D87631: [AVR] Fix global references to function symbols

[Dylan McKay via Phabricator via llvm-commits]

dylanmckay requested changes to this revision.
dylanmckay added a comment.
This revision now requires changes to proceed.

Good bugfix - thanks for the second patch!



================
Comment at: llvm/lib/Target/AVR/AVRAsmPrinter.cpp:187
+    bool IsFunction = isa<Function>(GV);
+    if (IsFunction) {
+      const MCExpr *Expr = MCSymbolRefExpr::create(getSymbol(GV), Ctx);
----------------
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 


================
Comment at: llvm/lib/Target/AVR/MCTargetDesc/AVRELFObjectWriter.cpp:11
 #include "MCTargetDesc/AVRMCTargetDesc.h"
+#include "MCTargetDesc/AVRMCExpr.h"
 
----------------
Order the `#include` alphabetically


================
Comment at: llvm/test/CodeGen/AVR/rust-trait-object.ll:10
+
+%"uno_traitobject2::A" = type {}
+
----------------
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.


================
Comment at: llvm/test/CodeGen/AVR/rust-trait-object.ll:65
+; Function Attrs: argmemonly nounwind willreturn
+declare void @llvm.lifetime.start.p0i8(i64 immarg, i8* nocapture) addrspace(1) #2
+
----------------
Remove attribute specifiers (`#n`) after functions - most if not all of them should be unnecessary


================
Comment at: llvm/test/CodeGen/AVR/rust-trait-object.ll:72
+; Function Attrs: noinline nounwind optsize
+define internal fastcc i8 @"uno_traitobject2::test::haeec7ec13f8f89c7"(i1 zeroext %choice) unnamed_addr addrspace(1) #3 {
+start:
----------------
Drop the linking options and calling convention `fastcc` - I suspect these are not needed to reproduce


================
Comment at: llvm/test/CodeGen/AVR/rust-trait-object.ll:141
+
+attributes #0 = { nounwind readnone speculatable willreturn }
+attributes #1 = { noreturn nounwind optsize "target-cpu"="atmega328p" }
----------------
Delete the attributes which are not required for your test to pass


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87631/new/

https://reviews.llvm.org/D87631