Page MenuHomePhabricator

[ARM][MachineOutliner] Do not overestimate LR liveness in return block
ClosedPublic

Authored by chill on Oct 10 2020, 8:20 AM.

Details

Summary

The LiveRegUnits utility (as well as LivePhysRegs) considers callee-saved registers to be alive at the point after the return instruction in a block. In the ARM backend, the LR register is classified as callee-saved, which is not really correct (from an ARM eABI or just common sense point of view).
These two conditions cause the MachineOutliner to overestimate the liveness of LR, which results in unnecessary saves/restores of LR around calls to outlined sequences.
It also causes the MachineVerifer to crash in some cases, because the save instruction reads a dead LR, for example when the following program:

int h(int, int);

int f(int a, int b, int c, int d) {
  a = h(a + 1, b - 1);
  b = b + c;
  return 1 + (2 * a + b) * (c - d) / (a - b) * (c + d);
}

int g(int a, int b, int c, int d) {
  a = h(a - 1, b + 1);
  b = b + c;
  return 2 + (2 * a + b) * (c - d) / (a - b) * (c + d);
}

is compiled with -target arm-eabi -march=armv7-m -Oz.

This patch computes the liveness of LR in return blocks only, while taking into account the few ARM instructions, which
read LR, but nevertheless the register is not mentioned (explicitly or implicitly) in the instruction operands.

Diff Detail

Event Timeline

chill created this revision.Oct 10 2020, 8:20 AM
chill requested review of this revision.Oct 10 2020, 8:20 AM
chill added reviewers: yroux, paquette, efriedma.
yroux added a comment.Oct 12 2020, 6:05 AM

Hi,

Thanks for raising this issue, I've two inlined comments

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
5677–5681

Can't we use MachineInstr::modifiesRegister() here ?

5694–5696

same question with MachineInstr::readsRegister()

chill updated this revision to Diff 298422.Oct 15 2020, 11:02 AM
chill marked 2 inline comments as done.
yroux added inline comments.Oct 16 2020, 5:53 AM
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
5677

I think we can get ride of passing nullptr since it is TRI default value in the prototype

5690

likewise

chill marked 2 inline comments as done.Oct 17 2020, 9:12 AM
chill added inline comments.
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
5677

One would think so, wouldn't they? :D

I created D89625

efriedma added inline comments.Oct 20 2020, 4:00 PM
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
5677

If you omit TRI, you'll miss defs of super-registers of LR. I'm not sure there are any at the moment, but there are for other GPRs.

chill updated this revision to Diff 299929.Oct 22 2020, 5:07 AM
chill marked an inline comment as done.
chill marked an inline comment as done.Oct 22 2020, 5:11 AM
chill added inline comments.
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
5677

Fair enough, there are odd/even GPR pairs that do not include LR, but I wouldn't write off the possibility of LR being part of a super-register in the future.

chill marked an inline comment as done.Nov 2 2020, 1:25 AM

Ping.

yroux accepted this revision.Nov 2 2020, 1:29 AM

LGTM

(Sorry for the delay, I took some vacations)

This revision is now accepted and ready to land.Nov 2 2020, 1:29 AM