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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

I'm generally worried about allowing unknown instructions here.

Any AArch64 instruction that produces a 32-bit result zeros the high bits, yes.  But some MachineInstr opcodes aren't really instructions in this sense.  You've noted EXTRACT_SUBREG, PHI, and COPY specifically.  Probably missing IMPLICIT_DEF.  Not sure if there are other relevant instructions; any pseudo-instruction is potentially an issue, but auditing them for AArch64, the target-specific ones mostly don't produce GPR32.

In any case, I'd be happier if we had a bit to check for a "real" instruction, in TSFlags or something like that.  I don't want to worry about modifying this in the future if, for example, we end up with a FREEZE MachineInstr.



================
Comment at: llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp:215
+  MachineInstr *SrcMI = MRI->getUniqueVRegDef(MI.getOperand(2).getReg());
+  if (SrcMI->getParent() == MI.getParent())
+    return false;
----------------
jaykang10 wrote:
> dmgreen wrote:
> > jaykang10 wrote:
> > > 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)>;
> > > ```
> > But, is that needed for this pass? I can see it is how DAG ISel works, but if there are this pattern of code in the same block, it should still work fine, shouldn't it?
> Yep, I agree with you. Let me remove the check for different block.
Missed update?


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

https://reviews.llvm.org/D110841