This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Fix offset calculation
ClosedPublic

Authored by smeenai on Oct 15 2019, 10:21 PM.

Details

Summary

r374772 changed Offset to be an int64_t but left NewOffset as an int,
which led to integer promotion issues in this calculation and resulted
in bad offset values. Promote NewOffset to int64_t as well to fix this,
and promote EmittableOffset as well, since its one user passes it to a
function which takes an int64_t anyway. This manifested as an
out-of-memory when building the Swift standard library for Android
aarch64. Test case suggested by Sander de Smalen!

Event Timeline

smeenai created this revision.Oct 15 2019, 10:21 PM

I have no idea how to write a test for this; I'm completely unfamiliar with backends. I have a Swift compilation command that reproduces the OOM mentioned in the commit message, and I think I should be able to get IR or MIR from that, but I'd appreciate guidance crafting a test case if it's considered necessary for this commit.

smeenai marked an inline comment as done.Oct 15 2019, 10:29 PM
smeenai added inline comments.
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
3442

EmittableOffset is still an int ... idk if I should be promoting that too.

smeenai marked an inline comment as done.Oct 15 2019, 10:36 PM
smeenai added inline comments.
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
3438

Just casting NewOffset to int64_t over here is actually sufficient, but I figured changing the type entirely was better/cleaner.

Thanks for fixing this @smeenai!

I have no idea how to write a test for this; I'm completely unfamiliar with backends. I have a Swift compilation command that reproduces the OOM mentioned in the commit message, and I think I should be able to get IR or MIR from that, but I'd appreciate guidance crafting a test case if it's considered necessary for this commit.

You can create a MIR test with a single instruction accessing a pre-allocated stackslot with a very large offset, and check that the offset is generated correctly.

e.g. the following MIR

---
name: D69018
tracksRegLiveness: true
fixedStack:
  - { id: 0, offset: 2147483648, size: 1}
body: |
  bb.0:
    $x0 = LDRXui %fixed-stack.0, 0
    RET_ReallyLR
...

compiles with your patch, but fails to complete (i.e. it keeps running) without it.

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
3442

That should be fine, because it will be used for the immediate field of an instruction.

smeenai marked an inline comment as done.Oct 16 2019, 9:35 AM
smeenai added inline comments.
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
3442

I decided to change it to int64_t as well, since the only call site which uses this parameter passes it to MachineOperand::ChangeToImmediate, which takes an int64_t parameter.

smeenai updated this revision to Diff 225249.Oct 16 2019, 9:39 AM
smeenai edited the summary of this revision. (Show Details)

Add test

Thanks for fixing this @smeenai!

I have no idea how to write a test for this; I'm completely unfamiliar with backends. I have a Swift compilation command that reproduces the OOM mentioned in the commit message, and I think I should be able to get IR or MIR from that, but I'd appreciate guidance crafting a test case if it's considered necessary for this commit.

You can create a MIR test with a single instruction accessing a pre-allocated stackslot with a very large offset, and check that the offset is generated correctly.

e.g. the following MIR

---
name: D69018
tracksRegLiveness: true
fixedStack:
  - { id: 0, offset: 2147483648, size: 1}
body: |
  bb.0:
    $x0 = LDRXui %fixed-stack.0, 0
    RET_ReallyLR
...

compiles with your patch, but fails to complete (i.e. it keeps running) without it.

Thanks for the test case! I added it.

sdesmalen added inline comments.Oct 16 2019, 10:05 AM
llvm/test/CodeGen/AArch64/framelayout-large-offset.mir
2

nit: I would probably add -run-pass=prologepilog, to reduce possible interference from other passes.

15

Sorry I didn't notice this earlier when I posted the example MIR, but these subs are wrong. The offset is at a positive distance from the SP, so it should use add here. If I change the offset from 2147483648 to 2147483632 that changes. So I expect some other changes are needed to fix this.

smeenai marked an inline comment as done.Oct 16 2019, 10:15 AM
smeenai added inline comments.
llvm/test/CodeGen/AArch64/framelayout-large-offset.mir
15

If I revert my change and r374772, the problem still persists, so that seems like a pre-existing issue?

smeenai updated this revision to Diff 225256.Oct 16 2019, 10:29 AM

Use -run-pass=prologepilog in test

smeenai marked 2 inline comments as done.Oct 16 2019, 10:30 AM
smeenai added inline comments.
llvm/test/CodeGen/AArch64/framelayout-large-offset.mir
15

Given that this is a pre-existing problem, would you be okay with submitting this to solve the OOM and then filing a bug for the incorrect offset calculation?

thegameg added inline comments.Oct 16 2019, 10:42 AM
llvm/test/CodeGen/AArch64/framelayout-large-offset.mir
2

I believe this is missing a | FileCheck %s to perform the actual checks.

sdesmalen added inline comments.Oct 16 2019, 10:47 AM
llvm/test/CodeGen/AArch64/framelayout-large-offset.mir
15

Sigh, I'm clearly having a bit of a senior moment here :) maxint is 2147483647 (forgot the -1 in my calculator). But with the fixed/updated offset, the result is the same with/without your patch, so we'll need something different to test it.

smeenai marked 3 inline comments as done.Oct 16 2019, 10:52 AM
smeenai added inline comments.
llvm/test/CodeGen/AArch64/framelayout-large-offset.mir
2

Oops, forgot to add that back after testing something. Thanks!

15

The test as it stands does infinite loop/OOM without my patch though and completes successfully after.

smeenai updated this revision to Diff 225280.Oct 16 2019, 11:37 AM
smeenai marked 2 inline comments as done.

Update test

smeenai marked 2 inline comments as done.Oct 16 2019, 11:38 AM

@sdesmalen How does the new test case look?

sdesmalen accepted this revision.Oct 16 2019, 1:29 PM

@sdesmalen How does the new test case look?

Thanks, the new test-case seems to cover the case well. It is out of range of the immediate and with NewOffset as int64_t the expression NewOffset * Scale should no longer be evaluated as unsigned.

LGTM

llvm/test/CodeGen/AArch64/framelayout-large-offset.mir
3

nit: You'll want to change the name of the test/function now.

This revision is now accepted and ready to land.Oct 16 2019, 1:29 PM
smeenai marked an inline comment as done.Oct 16 2019, 1:32 PM

Thanks for the quick review!

llvm/test/CodeGen/AArch64/framelayout-large-offset.mir
3

Yup, will change before committing.

This revision was automatically updated to reflect the committed changes.