This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Bail out on calls to dllimported functions
ClosedPublic

Authored by mstorsjo on Jan 26 2018, 1:37 AM.

Diff Detail

Event Timeline

mstorsjo created this revision.Jan 26 2018, 1:37 AM

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

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.

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 updated this revision to Diff 131563.Jan 26 2018, 2:46 AM
mstorsjo retitled this revision from [AArch64] Don't enable GlobalISel automatically for windows to [GlobalISel] Bail out on calls to dllimported functions.
mstorsjo edited the summary of this revision. (Show Details)

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

aemerson accepted this revision.Jan 26 2018, 7:59 AM

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

This revision is now accepted and ready to land.Jan 26 2018, 7:59 AM
mstorsjo added a subscriber: hans.Jan 30 2018, 11:53 AM

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

This revision was automatically updated to reflect the committed changes.
hans added a comment.Jan 31 2018, 1:02 AM

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

r323853.