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

[Geoff Berry via Phabricator via llvm-commits]

gberry marked 5 inline comments as done.
gberry 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) {
----------------
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.


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:260
+      UseI.getRegClassConstraint(UseIdx, TII, TRI))
+    return URC->contains(CopySrcReg);
+
----------------
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?


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:346
+    // semantics.
+    if (MRI->isReserved(UseReg))
+      continue;
----------------
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.


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:386
+    if (!CopySrc.isRenamable())
+      MOUse.setIsRenamable(false);
+
----------------
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.


https://reviews.llvm.org/D41835