This is an archive of the discontinued LLVM Phabricator instance.

Fix incorrect choice of callee-saved registers save/restore points
ClosedPublic

Authored by chill on Apr 11 2018, 9:14 AM.

Details

Summary

So, I have this testcase:

void f(int n, int x[]) {
  if (n < 0)
    return;

  int a[n];

  for (int i = 0; i < n; i++)
    a[i] = x[n - i - 1];

  for (int i = 0; i < n; i++)
    x[i] = a[i] + 1;
}

that, compiled with -O1/-Os for AArch64 and X86, generates machine code, which
fails to properly restore the stack pointer upon function return.

The testcase allocates a VLA, thus clang generates calls to llvm.stacksave and
llvm.stackrestore. The latter call is lowered to mov sp, x2, however this
move does not affect decisions made by the shrink wrapping pass, as the
instruction does not use or define a callee-saved register.

The end effect is that placement of register save/restore code is such that
along a certain path, the callee-saved registers and the stack pointer are
restored, but then the stack pointer is overwritten with an incorrect value.

This patches fixes the issue this by modifying ShrinkWrap::useOrDefCSROrFI to explicitly check
for the stack pointer register (using TLI.getStackPointerRegisterToSaveRestore) in non-call instructions.

Diff Detail

Event Timeline

chill created this revision.Apr 11 2018, 9:14 AM
chill added inline comments.Apr 11 2018, 9:55 AM
test/CodeGen/Generic/shrink-wrapping-vla.ll
88

I saw this one :/

Thanks for this! If it's possible, could you also write a MIR test for this?

lib/CodeGen/ShrinkWrap.cpp
262

I think this is incorrect if the instruction calls a function with a different calling convention, right? In that case some CSRs would be clobbered while otherwise they were expected to be preserved.

IIUC, the issue here is that some instructions are marked as let Uses = [SP], like calls and returns, but are safe to skip during the analysis.

If I'm not missing anything, to handle all the cases I think it's better to verify all the register operands and reg masks, then only skip the instruction if it only uses SP and isCall or something similar. I'm not sure if isCall would handle all the cases, but from a quick glance over *InstrInfo.td, other instructions that have Uses = [SP] should definitely affect the placement of the save/restore blocks.

junbuml added inline comments.Apr 11 2018, 10:57 AM
lib/CodeGen/ShrinkWrap.cpp
265

Why don't we make it as a member variable?

chill updated this revision to Diff 142437.Apr 13 2018, 10:38 AM
chill marked 3 inline comments as done.Apr 13 2018, 10:42 AM
chill added inline comments.
lib/CodeGen/ShrinkWrap.cpp
262

Thanks, indeed. For register masks, if I understand correctly they are used with call instructions and these should preserve SP, so no need to actually check it ?

265

Right, I made the stack pointer register number a member variable.

thegameg accepted this revision.Apr 15 2018, 4:18 AM

Thanks. LGTM + one more comment.

lib/CodeGen/ShrinkWrap.cpp
277–278

Can you please add a comment explaining why we skip SP here?

This revision is now accepted and ready to land.Apr 15 2018, 4:18 AM
chill updated this revision to Diff 142655.Apr 16 2018, 9:58 AM
chill marked 2 inline comments as done.

Added a comment; reformatted a little

chill marked an inline comment as done.Apr 16 2018, 9:59 AM

Thanks a lot for the comments and the review.

thegameg added inline comments.Apr 16 2018, 5:20 PM
lib/CodeGen/ShrinkWrap.cpp
280

Small typo: is -> as?

chill updated this revision to Diff 142745.Apr 17 2018, 1:28 AM
chill marked an inline comment as done.
chill edited the summary of this revision. (Show Details)Apr 17 2018, 1:39 AM
This revision was automatically updated to reflect the committed changes.

IIRC, there was a test case in the original patch but I can't see it in the landed commit. Any idea what happened?

chill added a comment.Apr 17 2018, 8:59 AM

IIRC, there was a test case in the original patch but I can't see it in the landed commit. Any idea what happened?

Oh, snap, I fail at svn ...