Page MenuHomePhabricator

[MachineOutliner][AArch64] Fix for noreturn functions
AbandonedPublic

Authored by plotfi on Jul 9 2020, 1:19 AM.

Details

Summary

Noreturn functions conservatively require save + restore LR.
When there is no available register, the stack is used to handle LR. If the block contains a call, the outlined function also needs to set a frame on the stack. This caused an assertion failure for OF.FrameConstructionID != MachineOutlinerDefault && "Can only fix up stack references once".
This checks whether a candidate is MachineOutlinerDefault while requiring the modification of the stack due to a call. If that is the case, outlining is bailed-out to avoid modifying the stack twice.

Diff Detail

Event Timeline

kyulee created this revision.Jul 9 2020, 1:19 AM
kyulee edited the summary of this revision. (Show Details)Jul 9 2020, 1:23 AM
kyulee added reviewers: paquette, tellenbach.

@kyulee Did you by chance come across this on the MultiSource/Applications/Burg test on llvm-test-suite? I hit something like this with -flto=full and -Oz and have been working on debugging it.

@plotfi I hit this in an internal workload (-Oz -flto=thin), but it won't surprise if the same issue is in llvm-test-suite.

@plotfi I hit this in an internal workload (-Oz -flto=thin), but it won't surprise if the same issue is in llvm-test-suite.

This is really nice. It does fix the issue I hit; I just checked.

plotfi added inline comments.Jul 15 2020, 5:00 PM
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
6162

From what I can tell, bailing here with a RepeatedSequenceLocs.clear(); return outliner::OutlinedFunction(); also works. It seems to be if the candidate does not have an available LR or is noreturn, doesn't have available registers, and isn't using using the SP is is probably highly likely to get set to MachineOutlinerDefault unless the candidate size doesn't exceed the 12 bytes of a standard LR/BL/restore LR.

paquette added inline comments.Jul 15 2020, 5:48 PM
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
6162

You don't always want to bail out here.

You only want to bail out here when the candidates contain a call, or IsNoReturn == true.

This just says that we'll save LR to the stack.

6198–6205

I think this should only happen if we have MachineOutlinerDefault *and* the candidate requires stack fixups.

In the code above:

// Is SP used in the sequence at all? If not, we don't have to modify
// the stack, so we are guaranteed to get the same frame.
else if (C.UsedInSequence.available(AArch64::SP)) {
  NumBytesNoStackCalls += 12;
  C.setCallInfo(MachineOutlinerDefault, 12);
  CandidatesWithoutStackFixups.push_back(C);
}

I don't think you want to remove candidates like these.

Technically, the commit message is inaccurate here, because this is really checking for situations where we fix things up twice. noreturn functions are one such case.

plotfi added inline comments.Jul 15 2020, 5:50 PM
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
6162

Ah! Yeah, that makes sense. The case that I was hitting does have a call.

plotfi commandeered this revision.Aug 14 2020, 12:08 AM
plotfi abandoned this revision.
plotfi edited reviewers, added: kyulee; removed: plotfi.