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

[JinGu Kang via Phabricator via llvm-commits]

jaykang10 added a comment.

In D110841#3039405 <https://reviews.llvm.org/D110841#3039405>, @dmgreen wrote:

> 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.

At instruction selection level, we check the operations which do not zero-out the high half of the 64-bit register using `isDef32` function as below.

  // Any instruction that defines a 32-bit result zeros out the high half of the
  // register. Truncate can be lowered to EXTRACT_SUBREG. CopyFromReg may
  // be copying from a truncate. But any other 32-bit operation will zero-extend
  // up to 64 bits. AssertSext/AssertZext aren't saying anything about the upper
  // 32 bits, they're probably just qualifying a CopyFromReg.
  static inline bool isDef32(const SDNode &N) {
    unsigned Opc = N.getOpcode();
    return Opc != ISD::TRUNCATE && Opc != TargetOpcode::EXTRACT_SUBREG &&
           Opc != ISD::CopyFromReg && Opc != ISD::AssertSext &&
           Opc != ISD::AssertZext && Opc != ISD::AssertAlign &&
           Opc != ISD::FREEZE;
  }

As you can see, the `isDef32` checks fundamentally `ISD::TRUNCATE` within a basic block because the below pattern is matched with the `ISD::TRUNCATE` and `EXTRACT_SUBREG` does not guarantee the high 32 bits are zero.

  // To truncate, we can simply extract from a subregister.
  def : Pat<(i32 (trunc GPR64sp:$src)),
            (i32 (EXTRACT_SUBREG GPR64sp:$src, sub_32))>;

This patch checks the `ORRWrs` has `EXTRACT_SUBREG` in different basic block as operand because the existing pattern with `isDef32` resolves case in which the operand is in same basic block.

Let me try to add MIR tests.



================
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)
----------------
dmgreen wrote:
> Do we need to check for WZR?
You are right! We need to check it because there is below assembly pattern. Let me update the code.
```
def : InstAlias<"mov $dst, $src", (ORRWrs GPR32:$dst, WZR, GPR32:$src, 0), 2>;
```



================
Comment at: llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp:215
+  MachineInstr *SrcMI = MRI->getUniqueVRegDef(MI.getOperand(2).getReg());
+  if (SrcMI->getParent() == MI.getParent())
+    return false;
----------------
dmgreen wrote:
> Do we need to avoid when they are in the same block? It would be easier for testing if we didn't.
If the pattern is in same block, I thought it has already been handled by existing pattern with `isDef32` and we do not need to check it.
```
def def32 : PatLeaf<(i32 GPR32:$src), [{
  return isDef32(*N);
}]>;

// In the case of a 32-bit def that is known to implicitly zero-extend,
// we can use a SUBREG_TO_REG.
def : Pat<(i64 (zext def32:$src)), (SUBREG_TO_REG (i64 0), GPR32:$src, sub_32)>;
```


================
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)
----------------
dmgreen wrote:
> Why do we rule these out? Why don't we rule out anything else?
I was not sure it is good to keep track of the operands of PHI and COPY in this patch... because it could make code complicated... for example, checking cycled phi. If possible, I would like to solve it in separate patch...


================
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.
----------------
dmgreen wrote:
> -> definition
Sorry... let me update it.


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

https://reviews.llvm.org/D110841