[PATCH] D38530: [AArch64] Add support for dllimport of values and functions

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:3714
       Subtarget->ClassifyGlobalReference(GV, getTargetMachine());
+  EVT PtrVT = getPointerTy(DAG.getDataLayout());
+  SDValue Result;
----------------
compnerd wrote:
> Can you lower this to ~L3725.
Ok, moving all of these three new variables down closer to where they're set and used.


================
Comment at: lib/Target/AArch64/AArch64MCInstLower.cpp:41
+  const Triple &TheTriple = Printer.TM.getTargetTriple();
+  if (TheTriple.isOSBinFormatCOFF()) {
+    assert(TheTriple.isOSWindows() &&
----------------
compnerd wrote:
> Early return please:
> 
>     if (!TheTriple.isOSBinFormatCOFF())
>       return Printer.getSymbol(MO.getGlobal());
>     ...
Sure


================
Comment at: test/CodeGen/AArch64/dllimport.ll:1
+; RUN: llc -mtriple aarch64-windows -filetype asm -o - %s | FileCheck %s
+
----------------
compnerd wrote:
> I prefer the canonicalized triple `aarch64-unknown-windows-msvc`
Ok, will change


================
Comment at: test/CodeGen/AArch64/dllimport.ll:6
+declare dllimport arm_aapcs_vfpcc i32 @external()
+declare arm_aapcs_vfpcc i32 @internal()
+
----------------
compnerd wrote:
> Are the `arm_aapcs_vfpcc` correct here?  The variant of AAPCS64 isn't marked differently I thought.  The same through out.
Indeed, this is just a leftover since I based this on the ARM dllimport test - will remove.


https://reviews.llvm.org/D38530