[PATCH] D69018: [AArch64] Fix offset calculation

Wed Oct 16 10:56:02 PDT 2019

smeenai marked 3 inline comments as done.
smeenai added inline comments.

================
Comment at: llvm/test/CodeGen/AArch64/framelayout-large-offset.mir:14
+# CHECK:              sub     x8, sp, #4095, lsl #12
+# CHECK-NEXT:         sub     x8, x8, #4095, lsl #12
+# CHECK-NEXT:         sub     x8, x8, #4095, lsl #12
----------------
sdesmalen wrote:
> smeenai wrote:
> > smeenai wrote:
> > > sdesmalen wrote:
> > > > Sorry I didn't notice this earlier when I posted the example MIR, but these `sub`s 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.
> > > If I revert my change and r374772, the problem still persists, so that seems like a pre-existing issue?
> > 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?
> 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.
The test as it stands does infinite loop/OOM without my patch though and completes successfully after.

================
Comment at: llvm/test/CodeGen/AArch64/framelayout-large-offset.mir:1
+# RUN: llc -mtriple=aarch64-none-linux-gnu -run-pass=prologepilog %s -o -
+---
----------------
thegameg wrote:
> I believe this is missing a `| FileCheck %s` to perform the actual checks.
Oops, forgot to add that back after testing something. Thanks!

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69018/new/

https://reviews.llvm.org/D69018