[PATCH] D152828: [MachineSink][AArch64] Sink instruction copies when they can replace copy into hard register or folded into addressing mode

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

There is a lot of code here, but it looks like a good idea. I was looking at folding shifts into and/or recently too, and it looked like there were cases where it is better to be careful about where it is combined based on the uses. The same is likely true for add/sub and addressing modes too.



================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:511
+    LLVM_DEBUG(dbgs() << "Sinking copy of"; MI.dump(); dbgs() << "into";
+               SinkDst->dump(););
+    if (SinkDst->isCopy()) {
----------------
Doesn't need a ; to end the inside of the LLVM_DEBUG.


================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:535
+    } else {
+      // Fold instruction into the addressing mode a memory instruction.
+      New = TII->emitLdStWithAddr(*SinkDst, MaybeAM);
----------------
-> addressing mode memory instruction?


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:2863
+    if (Shift == 1 && !Subtarget.hasLSLFast())
+      return false;
+    return canFoldAddRegIntoAddrMode(1 << Shift);
----------------
I think it's worth clarifying how these should work with LSLFast and addressing operands. I had an explanation of how I thought LSLFast should work in https://reviews.llvm.org/D155470#4527270. Let me know if you think doesn't sound right. There is a patch to split LSLFast into multiple parts as a first step in https://reviews.llvm.org/D157982.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:3264
+      unsigned Opcode = regOffsetOpcode(MemI.getOpcode());
+      // Copy the base register to the correct register class.
+      Register BaseReg = MRI.createVirtualRegister(&AArch64::GPR64spRegClass);
----------------
Would constrainRegClass work too, to avoid the COPY?


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:9136
+    // 12-bit unsigned offset
+    unsigned shift = Log2_64(NumBytes);
+    if (NumBytes && Offset > 0 && (Offset / NumBytes) <= (1LL << 12) - 1 &&
----------------
shift -> Shift


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:2716
+
+  // Handle memory instructions with a [Reg, Reg] addtessing mode.
+  if (MemI.getOperand(2).isReg()) {
----------------
addressing


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:3283
+
+    // The new insrtruction will be in the form `ld[u]r Rt, [Xn, #imm]`.
+    unsigned Scale = 1;
----------------
instruction


================
Comment at: llvm/test/CodeGen/AArch64/arm64-xaluo.ll:105
 ; SDAG:       // %bb.0: // %entry
-; SDAG-NEXT:    mov w8, #16777215
+; SDAG-NEXT:    mov w8, #16777215 // =0xffffff
 ; SDAG-NEXT:    adds w8, w0, w8
----------------
Can you regenerate these tests in the parent, to remove the unrelated changes from here?


================
Comment at: llvm/test/CodeGen/AArch64/sink-and-fold.ll:305
-
-define i32 @f6(i1 %c, ptr %a, i32 %i) {
-; CHECK-LABEL: f6:
----------------
Are these tests new? Can this be removed in the parent?


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

https://reviews.llvm.org/D152828