Page MenuHomePhabricator

[ARM][MachineOutliner] Add default mode.
ClosedPublic

Authored by yroux on Mar 12 2020, 8:05 AM.

Details

Summary

Use the stack to save and restore the link register when there is no
available register to do it.

This is a follow-up of ARM Machine Outliner support RFC D57054 and
should be applied on top of D76068

Diff Detail

Event Timeline

yroux created this revision.Mar 12 2020, 8:05 AM

I think that there's a lot to be tested here which isn't covered yet. Maybe it would make sense to split this patch a bit further? E.g. factor out outlining calls into a separate patch?

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
5823–5840

This should have a testcase?

5977–5980

I wonder if this check should move to the generic outliner code, since it seems like every target requires it.

5995–5997

There should be a testcase for outlining calls?

6116–6122

There should be a testcase showing the save/restore from SP.

llvm/test/CodeGen/ARM/machine-outliner-nosave-and-regs.mir
67–83 ↗(On Diff #249940)

Does this testcase save + restore LR? It would be good to check for the save + restore (or lack thereof) explicitly instead of just checking for the outlined call.

yroux marked 5 inline comments as done.Mar 25 2020, 7:37 AM

Thanks for the review Jessica,

I'll split it into two part and add test cases

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
5823–5840

OK

5977–5980

hmm, yes maybe looking at it more closely for ARM it is in fact gnu_mcount_nc and mcount but _mcount is everywhere else and I think it only makes sense if we are in linux context (not needed for bare metal)

5995–5997

ok

6116–6122

It is tested in machine-outliner-nosave-and-regs.mir but maybe needs to put inside a dedicated testcase

llvm/test/CodeGen/ARM/machine-outliner-nosave-and-regs.mir
67–83 ↗(On Diff #249940)

yes it is, but you are right I'll explicitly checked the output

yroux updated this revision to Diff 279241.Jul 20 2020, 7:42 AM

I've extracted parts which handle calls inside outlined frames (I'll submit that in the next patch) and enhanced the testcases.

efriedma added inline comments.Jul 20 2020, 12:18 PM
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
6106

In general, calling addLiveIn(LR) on the current block isn't sufficient to update the liveness correctly. If LR is live-in in the current block, it has to be live-out of that block's predecessors, which means it has to be live-in to the current block's predecessors. See D78586.

Actually, can the isLiveIn() check fail? if LR isn't live-in, wouldn't we choose MachineOutlinerNoLRSave? Or is it possible to hit some weird case where LR is live, but not explicitly listed in the live-in list?

yroux added inline comments.Jul 23 2020, 5:30 AM
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
6106

Right, when a block ends with a BX_RET for instance, LR is not marked as live-in and if we outline a chunk before LR as to be saved and restored in a register or the stack, I'm pretty sure I had to add LR to the live-in set because the Machine Verifier wasn't happy of this explicit use, but I don't manage to reproduce that behavior...

I'll remove the check If that's not needed anymore

yroux updated this revision to Diff 280895.Jul 27 2020, 6:53 AM

Removed uneeded LR addition to live-in set.

This looks okay to me. Eli, do you have any other comments? For the BX_RET issue, could we work around it by making outlining illegal in its presence?

This revision is now accepted and ready to land.Aug 10 2020, 12:53 PM
This revision was landed with ongoing or failed builds.Aug 20 2020, 12:31 AM
This revision was automatically updated to reflect the committed changes.