[PATCH] D103597: [AArch64LoadStoreOptimizer] Generate more STPs by renaming registers earlier

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Besides some nits/observations inline, one question about testing. The reproducer that was attached does not get miscompiled anymore? And it is exactly one of these cases that was added, i.e. the modified base register? Just asking to see if there's not another case or test that we are missing.



================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:1526
+    const TargetRegisterInfo *TRI) {
+  if (DebugCounter::shouldExecute(RegRenamingCounter)) {
+    if (!MaybeCanRename)
----------------
Style nit: can we return early here to reduce indentation:

    if (!DebugCounter::shouldExecute(RegRenamingCounter))
      return false;


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:1697
+        if (TRI->isSuperOrSubRegisterEq(Reg, getLdStRegOp(MI).getReg()) &&
+            ModifiedRegUnits.available(BaseReg) &&
+            UsedRegUnits.available(BaseReg)) {
----------------
Diffing with previous revision does not seem to work, so just double checking this. 
You have added here the check if the base reg has been modified, and is available here, similarly like that is done from line 1726, right?

That seems okay to be me now.


================
Comment at: llvm/test/CodeGen/AArch64/consthoist-gep.ll:3
 
 ; CHECK-NOT: adrp    x10, global+332
 ; CHECK-NOT: add     x10, x10, :lo12:global+332
----------------
Unrelated nit: perhaps align the assembly of this block for readability, indentation is all over the place


================
Comment at: llvm/test/CodeGen/AArch64/stp-opt-with-renaming.mir:666
+  bb.0.entry:
+    liveins: $x0, $x1, $q1, $q2, $q3, $q4, $q5, $q6, $q7, $q8, $q9, $q10
+    renamable $q0 = LDRQui renamable $x1, 1 :: (load 16)
----------------
And here you have added a lot of registers as live-in, which results in no registers being available for renaming. Nice one, that looks good.


================
Comment at: llvm/test/CodeGen/AArch64/stp-opt-with-renaming.mir:701
+    STRQui killed renamable $q0, renamable $x0, 1 :: (store 16, basealign 32)
+    renamable $q0 = LDRQui killed renamable $x1, 0 :: (load 16, align 32)
+    STRHHui $wzr, renamable $x0, 12 :: (store 2, align 8)
----------------
Yep, and here the base register is modified. Looks good.


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

https://reviews.llvm.org/D103597