Page MenuHomePhabricator

[ARM] Glue register copies to tail calls.
ClosedPublic

Authored by efriedma on Apr 8 2019, 4:39 PM.

Details

Summary

This generally follows what other targets do. I don't completely understand why the special case for tail calls existed in the first place; even when the code was committed in r105413, call lowering didn't work in the way described in the comments.

Stack protector lowering breaks if the register copies are not glued to a tail call: we have to insert the stack protector check before the tail call, and we choose the location based on the assumption that all physical register dependencies of a tail call are adjacent to the tail call. (See FindSplitPointForStackProtector.) This is sort of fragile, but I don't see any reason to break that assumption.

I'm guessing nobody has seen this before just because it's hard to convince the scheduler to actually schedule the code in a way that breaks; even without the glue, the only computation that could actually be scheduled after the register copies is the computation of the call address, and the scheduler usually prefers to schedule that before the copies anyway.

Fixes https://bugs.llvm.org/show_bug.cgi?id=41417

Diff Detail

Repository
rL LLVM

Event Timeline

efriedma created this revision.Apr 8 2019, 4:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2019, 4:39 PM
dim added a comment.Apr 9 2019, 2:14 PM

I can confirm this fixes both the minimized test case from https://bugs.llvm.org/show_bug.cgi?id=41417, and the full original test case from https://bugs.freebsd.org/237074.

dmgreen accepted this revision.Apr 10 2019, 12:39 AM

LGTM then.

lib/Target/ARM/ARMISelLowering.cpp
1991 ↗(On Diff #194220)

for (auto &RegToPass : RegsToPass)

This revision is now accepted and ready to land.Apr 10 2019, 12:39 AM
dim added a comment.May 6 2019, 11:04 AM

@efriedma any more work to be done on this? :)

This revision was automatically updated to reflect the committed changes.