This is an archive of the discontinued LLVM Phabricator instance.

Fix function pointer tail calls in armv8-M.base
ClosedPublic

Authored by pbarrio on Dec 1 2017, 2:34 AM.

Details

Summary

The compiler fails with the following error message:

fatal error: error in backend: ran out of registers during
register allocation

Tail call optimization for Armv8-M.base fails to meet all the required
constraints when handling calls to function pointers where the
arguments take up r0-r3. This is because the pointer to the
function to be called can only be stored in r0-r3, but these are
all occupied by arguments. This patch makes sure that tail call
optimization does not try to handle this type of calls.

Event Timeline

pbarrio created this revision.Dec 1 2017, 2:34 AM
olista01 requested changes to this revision.Dec 1 2017, 7:07 AM

Could you also add a test for the (i32, i64) case? We can currently emit the tail call for that (because r1 is not used).

lib/Target/ARM/ARMISelLowering.cpp
2289

Why does it matter that the callee comes from a load? This code also triggers the error, without needing to load the function pointer:

define void @_Z3fooPFviiiiE(void (i32, i32, i32, i32)* nocapture %bar) local_unnamed_addr #0 {
entry:
  tail call void %bar(i32 0, i32 0, i32 0, i32 0)
  ret void
}
test/CodeGen/ARM/v8m-tail-call.ll
55

These tests should check that a blx is being emitted.

This revision now requires changes to proceed.Dec 1 2017, 7:07 AM
pbarrio updated this revision to Diff 125164.Dec 1 2017, 10:06 AM
pbarrio edited edge metadata.

Use !isa<GlobalAddressSDNode> to check if the call is through a function pointer

A non-pointer function call will always use a GlobalAddressSDNode; checking this
guarantees that the condition is met for all function pointers. Using
isa<LoadSDNode> only works when the pointer is dereferenced right before doing
the call, which only fixed part of the problem.

pbarrio marked 2 inline comments as done.Dec 1 2017, 10:08 AM

I have also updated the existing tests and added a couple more.

Why are we running out of registers here? We should copy the function pointer to r12, like we do for Thumb2 or ARM.

On a sort-of-related note, we currently miscompile the following:

void a(int (*p)(int,int,int,int)) {
  register int (*x)(int,int,int,int) asm("r0") = p;
  asm("ldr r0, =g":"=r"(x));
  x(1,2,3,4);
}
efriedma accepted this revision.Dec 1 2017, 2:15 PM

Looked a bit more; we're running out of registers because we don't consider r12 an allocatable register in Thumb1 mode. Changing that would be a project way outside the scope of this bugfix, so this is fine as-is for now. LGTM.

@efriedma , @olista01 , thank you for the reviews. I will commit upstream now.

olista01 accepted this revision.Dec 4 2017, 8:32 AM
This revision is now accepted and ready to land.Dec 4 2017, 8:32 AM
This revision was automatically updated to reflect the committed changes.