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

[Geoff Berry via Phabricator via llvm-commits]

gberry added a comment.

I've uploaded a new version that addresses most of the issues brought up by @qcolombet.

@arsenm could you take a look at the AMDGPU part of this change?



================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:386
+    if (!CopySrc.isRenamable())
+      MOUse.setIsRenamable(false);
+
----------------
qcolombet wrote:
> 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.
I'm not sure exactly what you're suggesting we add to the machine verifier here.


Repository:
  rL LLVM

https://reviews.llvm.org/D41835