[PATCH] D110841: [AArch64] Remove redundant ORRWrs which is generated by zero-extend

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Can you explain in more details what makes this valid? Does it depend on the top bits already being zero? What verifies that?

It might be useful to add mir tests too, to test specific cases that should/shouldn't be removed.



================
Comment at: llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp:209
+  // def : Pat<(i64 (zext GPR32:$src)),
+  //           (SUBREG_TO_REG (i32 0), (ORRWrs WZR, GPR32:$src, 0), sub_32)>;
+  if (MI.getOperand(3).getImm() != 0)
----------------
Do we need to check for WZR?


================
Comment at: llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp:215
+  MachineInstr *SrcMI = MRI->getUniqueVRegDef(MI.getOperand(2).getReg());
+  if (SrcMI->getParent() == MI.getParent())
+    return false;
----------------
Do we need to avoid when they are in the same block? It would be easier for testing if we didn't.


================
Comment at: llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp:228
+  // Conservatively, do not remove ORR from below MIs.
+  if (SrcMI->getOpcode() == TargetOpcode::PHI ||
+      SrcMI->getOpcode() == TargetOpcode::COPY)
----------------
Why do we rule these out? Why don't we rule out anything else?


================
Comment at: llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp:234
+  MRI->replaceRegWith(DefReg, MI.getOperand(2).getReg());
+  // replaceRegWith changes MI's defintion register. Keep it for SSA form until
+  // deleting MI.
----------------
-> definition


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

https://reviews.llvm.org/D110841