[GlobalISel] Bail out on calls to dllimported functions

[kristof.beyls]

Hi Martin,

Do I understand correctly that this patch both has code to disable globalisel when targeting windows, and also when a function is marked as "dllimport"?
Wouldn't one of these checks be enough?
From your description, I would have thought that just the "dllimport" check would be enough?
I'm probably missing quite a bit of background info here - but I thought I'd ask to understand this better.

Thanks,

Kristof

[mstorsjo]

In D42568#988786, @kristof.beyls wrote:

Hi Martin,

Do I understand correctly that this patch both has code to disable globalisel when targeting windows, and also when a function is marked as "dllimport"?
Wouldn't one of these checks be enough?
From your description, I would have thought that just the "dllimport" check would be enough?
I'm probably missing quite a bit of background info here - but I thought I'd ask to understand this better.

Just disabling globalisel for individual function calls to dllimport functions would indeed be the most elegant solution. However, the "return false;" in translateCall doesn't cause such fallbacking, but insteads makes the build abort with this error message:

LLVM ERROR: unable to translate instruction: call (in function: call_external)

So that part is kept as a reminder/check to whoever manually enables globalisel on windows, that the output isn't right in those cases. The generic disabling makes sure we don't trigger this and don't error out. If we on the other hand could do partial fallbacks to SelectionDAG for those function calls, that'd be even better.

[mstorsjo]

In D42568#988803, @mstorsjo wrote:

However, the "return false;" in translateCall doesn't cause such fallbacking, but insteads makes the build abort with this error message:

LLVM ERROR: unable to translate instruction: call (in function: call_external)

Actually, this was just a result of me testing back and forth with it disabled in general and enabled via the -global-isel switch. If enabled automatically, it silently falls back and doesn't error out. Will update the patch accordingly.

[mstorsjo]

Don't skip GlobalISel altogether, but just bail out on dllimport function calls.

[aemerson]

LGTM from a GISel point of view, I can't say I understand Windows enough to say the tests are right.

[mstorsjo]

@hans And this one also would be good to backport.

[hans]

In D42568#992232, @mstorsjo wrote:

@hans And this one also would be good to backport.

r323853.

lib/CodeGen/GlobalISel/IRTranslator.cpp

bool IRTranslator::translateInlineAsm(const CallInst &CI,
   return true;
 }
 
 bool IRTranslator::translateCall(const User &U, MachineIRBuilder &MIRBuilder) {
   const CallInst &CI = cast<CallInst>(U);
   auto TII = MF->getTarget().getIntrinsicInfo();
   const Function *F = CI.getCalledFunction();
 
+  // FIXME: support Windows dllimport function calls.
+  if (F && F->hasDLLImportStorageClass())
+    return false;
+
   if (CI.isInlineAsm())
     return translateInlineAsm(CI, MIRBuilder);
 
   Intrinsic::ID ID = Intrinsic::not_intrinsic;
   if (F && F->isIntrinsic()) {
     ID = F->getIntrinsicID();
     if (TII && ID == Intrinsic::not_intrinsic)
       ID = static_cast<Intrinsic::ID>(TII->getIntrinsicID(F));

lib/Target/AArch64/AArch64TargetMachine.cpp

AArch64TargetMachine::AArch64TargetMachine(const Target &T, const Triple &TT,
     : LLVMTargetMachine(T,
                         computeDataLayout(TT, Options.MCOptions, LittleEndian),
                         TT, CPU, FS, Options, getEffectiveRelocModel(TT, RM),
                         getEffectiveCodeModel(TT, CM, JIT), OL),
       TLOF(createTLOF(getTargetTriple())), isLittle(LittleEndian) {
   initAsmInfo();
 
   // Enable GlobalISel at or below EnableGlobalISelAt0.
-  if (getOptLevel() <= EnableGlobalISelAtO)
+  // Don't enable it automatically on Windows, where some vital
+  // concepts (calling of dllimport functions) haven't yet been hooked up.
+  if (getOptLevel() <= EnableGlobalISelAtO && !TT.isOSWindows())
     setGlobalISel(true);
 }
 
 AArch64TargetMachine::~AArch64TargetMachine() = default;
 
 const AArch64Subtarget *
 AArch64TargetMachine::getSubtargetImpl(const Function &F) const {
   Attribute CPUAttr = F.getFnAttribute("target-cpu");

test/CodeGen/AArch64/dllimport.ll

 ; RUN: llc -mtriple aarch64-unknown-windows-msvc -filetype asm -o - %s | FileCheck %s -check-prefixes=CHECK,DAG-ISEL
 ; RUN: llc -mtriple aarch64-unknown-windows-msvc -fast-isel -filetype asm -o - %s | FileCheck %s -check-prefixes=CHECK,FAST-ISEL
+; RUN: llc -mtriple aarch64-unknown-windows-msvc -O0 -filetype asm -o - %s | FileCheck %s -check-prefixes=CHECK,FAST-ISEL
 
 @var = external dllimport global i32
 @ext = external global i32
 declare dllimport i32 @external()
 declare i32 @internal()
 
 define i32 @get_var() {
   %1 = load i32, i32* @var, align 4