This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Fix constant islands pass.
ClosedPublic

Authored by rogfer01 on Feb 21 2017, 9:05 AM.

Details

Summary

The pass tries to fix a spill of LR that turns out to be unnecessary.
So it removes the tPOP but forgets to remove tPUSH.
This causes the stack be misaligned upon returning the function.

Thus, remove the tPUSH as well in this case.

The test looks a bit long but this problem is only observed in relatively
large functions requiring a large number of literals and a large
number of near branches. This test was reduced using bugpoint and
cannot be reduced further.

Diff Detail

Repository
rL LLVM

Event Timeline

rogfer01 created this revision.Feb 21 2017, 9:05 AM
rengolin accepted this revision.Feb 21 2017, 9:47 AM

Ouch, ok, that's not good.

The fix looks obvious, and the test is large, but given the intricate nature of the problem (push lr only happens unnaturally inside the compiler), I'm not sure what to recommend other than "go for it".

If you find a better reproducer, please update the test. Otherwise, it shouldn't matter too much.

cheers,
--renato

This revision is now accepted and ready to land.Feb 21 2017, 9:47 AM

Artificial testcase:

int a(int *z, int c) {
  unsigned pow = c;
#pragma unroll(2000)
  for (int i = 0; i < 2000; ++i)
    pow *= c; return pow;
}

The content of the function doesn't actually matter; you just need a large function which doesn't spill. See ARMFrameLowering::determineCalleeSaves.

Artificial testcase:

int a(int *z, int c) {
  unsigned pow = c;
#pragma unroll(2000)
  for (int i = 0; i < 2000; ++i)
    pow *= c; return pow;
}

That's still going to look horrendous in IR, no?

That's still going to look horrendous in IR, no?

We could run the testcase through the loop unroller before passing it to llc? Maybe that's a little too tricky.

jmolloy edited edge metadata.Feb 22 2017, 12:21 AM

Can we use the @llvm.space intrinsic here to replace most of this code? or will that change the constantislands behaviour?

This revision was automatically updated to reflect the committed changes.