[PATCH] D110841: [AArch64] Remove redundant ORRWrs which is generated by zero-extend
Eli Friedman via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Oct 15 13:05:00 PDT 2021
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
More information about the llvm-commits
mailing list