[PATCH] D70450: [AArch64] Teach Load/Store optimizier to rename store operands for pairing.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D70450#1832697 <https://reviews.llvm.org/D70450#1832697>, @hans wrote:

> > Thanks for sharing that (and sorry for any inconvenience caused). I'm looking forward to the reproducer. Please let me know if there's anything I can do to help.
>
> We now have a diff of the code before and after this patch. I'm not that familiar with aarch64. Could you take a look and see if this makes sense: https://bugs.chromium.org/p/chromium/issues/detail?id=1037912#c70 (the full Machine IR dumps are attached to the comment before)


That's great thanks! I think I know what's going on (I'll post here as I don't have a sign-in for bugs.chromium.org).

In the problematic case it looks like we turn

  renamable $w8 = KILL killed renamable $w8, implicit-def $x8
  STURXi killed renamable $x8, $fp, -40 :: (store 8 into %stack.5)
  $w8 = ORRWrs $wzr, killed $w25, 0, implicit-def $x8
  STURXi killed renamable $x8, $fp, -32 :: (store 8 into %stack.4)

into

  $w9 = KILL killed renamable $w8, implicit-def $x9
  $w8 = ORRWrs $wzr, killed $w25, 0, implicit-def $x8
  STPXi killed $x9, killed renamable $x8, $fp, -5 :: (store 8 into %stack.5), (store 8 into %stack.4)

On the MIR level, that seems fine, but the problem is that KILL is a noop, so it won't get lowered to any instructions, so `$w9` will not contain the correct value. (The changed -5 offset should be fine, as the 64 bit variant of STP uses multiples of 8)

IIUC there are multiple ways to fix this:

1. Change the operand flags on KILL. It seems like even though `$w8` is marked as renamable we cannot freely rename the result operand. Unless I am missing something, all operands should probably tied.
2. Add an additional copy to ensure we actually do the renaming.

If marking the operands of KILL as tied makes sense, I think that's what we should go for, as it is more general.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70450