X86FastISel(llc -O0) does not handle calling dllimport functions properly

[llvmbot]

Bugzilla Link	6275
Resolution	FIXED
Resolved on	Feb 20, 2011 23:58
Version	trunk
OS	Windows NT
Blocks	llvm/llvm-bugzilla-archive#8390
Reporter	LLVM Bugzilla Contributor
CC	@efriedma-quic,@sunfishcode

Extended Description

In investigating #6641 , we stumbled onto some very weird behavior regarding codegen of calls to dllimport functions.

Running Amine's example from #6641 :

attribute((dllimport)) void foo(int);
void bar(void) {foo(0);}

through clang with:

clang -cc1 -triple=i386-mingw32 -S dllimp.c

produces:

.def     _bar;  .scl    2;  .type   32; .endef
.text
.globl  _bar
.align  16

_bar:
subl $4, %esp
movl $0, (%esp)
call _foo
addl $4, %esp
ret

.def     _foo;  .scl    2;  .type   32; .endef

But oddly enough, there's nothing wrong with the LLVM IR that clang is generating:

; ModuleID = 'test-dll.c'
target datalayout = "e-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-f80:128:128-v64:64:64-v128:128:128-a0:0:64-f80:32:32-n8:16:32"
target triple = "i386-mingw32"

define void @​bar() nounwind {
entry:
call void @​foo(i32 0)
ret void
}

declare dllimport void @​foo(i32)

And compiling this to x86 assembly with llc produces the correct output:

.def     _bar;  .scl    2;  .type   32; .endef
.text
.globl  _bar
.align  16

_bar: # @​bar

BB#0: # %entry

subl    $4, %esp
movl    $0, (%esp)
call    *__imp__foo
addl    $4, %esp
ret


.def     _foo;  .scl    2;  .type   32; .endef

If I had a mingw32 version of llvm-gcc, I'd test it with that too just for comparison, but I don't.

[efriedma-quic]

I think this is a fast-isel bug; IR from comment 0 compiles correctly with llc -O1, but not llc -O0.

[sunfishcode]

Seems likely. X86FastISel::X86SelectCall probably just needs to check for it and return false.

[llvmbot]

Fixed in r116184.
Now we can see "declare dllimport void @​foo(i32)" and "calll *__imp__foo" with clang.

[efriedma-quic]

Err, for what definition of "working"? Testcase is still broken for me.

[llvmbot]

Err, for what definition of "working"? Testcase is still broken for me.

Oh I am sorry and thank you to reopen this.
I missed trying "exact" testcase.

r116184 was not for this PR, but it fixed "dllimport variable" on clang.
FYI there is no problem about dllimport variables with -O0.

extern attribute((dllimport)) int qux;
int quux(void) {return qux;}

_quux:
movl __imp__qux, %eax
movl (%eax), %eax
ret

[llvmbot]

Trivial fix. I have not tested widely yet.
Function cannot be GlobalVariable.
and I think latter "GVar" might be dubious.

--- a/lib/Target/X86/X86FastISel.cpp
+++ b/lib/Target/X86/X86FastISel.cpp
@@ -638,6 +638,10 @@ bool X86FastISel::X86SelectCallAddress(const Value *V, X86AddressMode &AM) {
(AM.Base.Reg != 0 || AM.IndexReg != 0))
return false;

• // Can't handle DLLImport.
• if (GV->hasDLLImportLinkage())

•  return false;
  

• // Can't handle TLS or DLLImport.
  if (const GlobalVariable *GVar = dyn_cast(GV))
  if (GVar->isThreadLocal() || GVar->hasDLLImportLinkage())

[llvmbot]

Trivial fix. I have not tested widely yet.
Function cannot be GlobalVariable.
and I think latter "GVar" might be dubious.

--- a/lib/Target/X86/X86FastISel.cpp
+++ b/lib/Target/X86/X86FastISel.cpp
@@ -638,6 +638,10 @@ bool X86FastISel::X86SelectCallAddress(const Value *V,
X86AddressMode &AM) {
(AM.Base.Reg != 0 || AM.IndexReg != 0))
return false;

• // Can't handle DLLImport.
• if (GV->hasDLLImportLinkage())

•  return false;
  

• // Can't handle TLS or DLLImport.
  if (const GlobalVariable *GVar = dyn_cast(GV))
  if (GVar->isThreadLocal() || GVar->hasDLLImportLinkage())

Yep, the old check just seems to be a typo, the hasDLLImportLinkage() check should apply to all GlobalValues, not just GlobalVariables.

One observation: Do we need to also add a similar check to X86SelectAddress()? It currently has:

// Can't handle TLS yet.
if (const GlobalVariable *GVar = dyn_cast<GlobalVariable>(GV))
  if (GVar->isThreadLocal())
    return false;

[llvmbot]

One observation: Do we need to also add a similar check to X86SelectAddress()?
It currently has:

// Can't handle TLS yet.
if (const GlobalVariable *GVar = dyn_cast<GlobalVariable>(GV))
  if (GVar->isThreadLocal())
    return false;

X86SelectAddress() already seems to have handling for MO_DLLIMPORT, so ignore that part of my comment.

[llvmbot]

Fixed in r126110.

    .def     _bar;
    .scl    2;
    .type   32;
    .endef
    .text
    .globl  _bar
    .align  16, 0x90

_bar: # @​bar

BB#0: # %entry

    pushl   %eax
    movl    %esp, %eax
    movl    $0, (%eax)
    movl    __imp__foo, %eax
    calll   *%eax
    popl    %eax
    ret

[llvmbot]

mentioned in issue llvm/llvm-bugzilla-archive#8390