[PATCH] D135957: [AArch64][SeperateConstOffsetFromGEP] Prevent pass from splitting GEP if an index has more than one use

[Zain Jaffal via Phabricator via llvm-commits]

zjaffal added a comment.

> There is a RISCV test that is failing.

I checked that out and the pass is correct for RISCV. I am thinking of moving this to be handled in AArch64 backend instead.

> Can you explain more why this is the correct fix? Why is it limited to 2 operand geps?

This is a correct fix for AArch64 because when we have a GEP with one operand and we split it we just end up generating extra instructions.
Taking the following block from the testcase:

  %tmp1 = load i64, i64* %base, align 8
  %tmp22 = add nsw i64 %tmp1, -1
  %tmp23 = getelementptr inbounds i64, i64* %arg, i64 %tmp22
  %tmp24 = load i64, i64* %tmp23, align 2
  br label %bb25

After the pass it will be this

  %0 = bitcast i64* %arg to i8*
  %1 = shl i64 %tmp1, 3
  %uglygep = getelementptr i8, i8* %0, i64 %1
  %uglygep2 = getelementptr i8, i8* %uglygep, i64 -8
  %2 = bitcast i8* %uglygep2 to i64*
  %tmp24 = load i64, i64* %2, align 2
  br label %bb25

`%1 = shl i64 %tmp1, 3` is unnecessary here and the backend fails to fold it into the load so we get the following assembly

  ldr	x9, [x1]
  mov	w8, #1
  add	x10, x0, x9, lsl #3
  sub	x9, x9, #1
  ldur	x10, [x10, #-8]

While disabling the pass will generate better assembly

  ldr	x8, [x1]
  mov	w10, #1
  sub	x8, x8, #1
  ldr	x9, [x0, x8, lsl  #3]

> Do you have benchmark results for changing it?

The patch increased the performance by ~5% on Python workloads in a proprietary set of benchmarks. I will test it on SPEC2017 and check the impact of the fix



================
Comment at: llvm/test/CodeGen/AArch64/aarch64-loop-gep-opt.ll:60
+
+  %tmp1 = load i64, i64* null, align 8
+  %tmp22 = add nsw i64 %tmp1, -1
----------------
dmgreen wrote:
> Should this be loading from nullptr?
It is necessary, I think it is better to move `%tmp1` to be a function argument instead


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135957