[PATCH] D41835: [MachineCopyPropagation] Extend pass to do COPY source forwarding

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:251
+/// in \param UseI based on the register class constraints.
+bool MachineCopyPropagation::isForwardableRegClassCopy(
+    const MachineInstr &Copy, const MachineInstr &UseI, unsigned UseIdx) {
----------------
gberry wrote:
> qcolombet wrote:
> > It looks like this function is meant to run only on allocated code.
> > If that's the case, say it in the comment.
> This pass is meant to only run on allocated code.  There is already a NoVRegs pass property check and an assert in CopyPropagateBlock that checks for virtual registers.  I'll add 'physical' to the comment.
I thought we were running before the rewriter, but I guess we don't need that anymore since we have the renamable flag.
Thanks for pointing that out.


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:260
+      UseI.getRegClassConstraint(UseIdx, TII, TRI))
+    return URC->contains(CopySrcReg);
+
----------------
gberry wrote:
> qcolombet wrote:
> > Unless you expect CopySrcReg to always be a physical register, this check needs to be adapted.
> As noted above, I do expect it to always be a physical register.  I can add asserts here, or perhaps rename the function to reduce confusion?
No given the pass itself doesn't expect vreg, we're fine.


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:346
+    // semantics.
+    if (MRI->isReserved(UseReg))
+      continue;
----------------
gberry wrote:
> qcolombet wrote:
> > I expect reserved registers shouldn't be renamable.
> > Am I missing something?
> > 
> > edit: Oh you may introduce this after forwarding a reserved register, if I am reading the code correctly. Then, unless I am missing something again, we should be fine rewriting this one as well.
> I believe this is a redundant check with the isRenamable that is there from the previous version.  I'll confirm by doing some testing with this changed to an assert before removing it.
> 
> edit: After trying this out, it appears that there are a few cases of targets generating renamable reserved registers.  One specific case is X86 and float-stack registers.  I'll try adding this check to the verifier to see if we can flush out all these cases before committing this change.
Sounds good


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:386
+    if (!CopySrc.isRenamable())
+      MOUse.setIsRenamable(false);
+
----------------
gberry wrote:
> qcolombet wrote:
> > Hmm, I have mixed feeling about this.
> > Is renamable a property of the register or of the machine operand?
> > 
> > If this is the machine operand, then this particular use of the register will still be renamable.
> > 
> > What I am saying is depending on the semantic we give to renamable, this change makes sense or not.
> If renamability is only a property of the opcode, then I don't think we need it, since we could always just check the opcode property.
> My thinking is that it applies more to the individual physical register, and implies the value needs to be in that particular register, whether due to opcode constraints, ABI constraints, inline asm constrains, GC-interaction constraints, etc.
In that case given we're really renaming the operand, I would think it should stay this way, but again, we could argue in both ways.

We should just agree what we're modeling with that flag and verify that in the machine verifier.


https://reviews.llvm.org/D41835