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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

Hi Geoff,

Looks mostly good for the generic parts.

Comments below.

Cheers,
-Quentin



================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:249
 
+/// Decide whether we should forward the destination of \param Copy to its use
+/// in \param UseI based on the register class constraints.
----------------
You mean the source of Copy, right?


================
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) {
----------------
It looks like this function is meant to run only on allocated code.
If that's the case, say it in the comment.


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:260
+      UseI.getRegClassConstraint(UseIdx, TII, TRI))
+    return URC->contains(CopySrcReg);
+
----------------
Unless you expect CopySrcReg to always be a physical register, this check needs to be adapted.


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:262
+
+  if (UseI.isCopy()) {
+    /// COPYs don't have register class constraints, so if the user instruction
----------------
Nit: to reduce indentation
if (!UseI.isCopy())
   return false;


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:288
+      SuperRC = *SuperRCI++;
+    } while (SuperRC);
+    return false;
----------------
Just personal taste but I feel like a for loop would be more natural here.
for (const TargetRegisterClass *SuperRC = UseDstRC, TargetRegisterClass::sc_iterator SuperRCI = UseDstRC->getSuperClasses(), SuperRC; SuperRC = *SuperRCI++)
        if (SuperRC->contains(CopySrcReg))
        return true;

(Technically, I admit this has one more check, but I believe it doesn't matter for performance.)


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


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:360
+    if (MOUse.getReg() != CopyDstReg)
+      continue;
+
----------------
Add a DEBUG message (and maybe an optimization remarks) for that case to see how often it happens and thus, if it is worth fixing.

Could be a follow-up patch.


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:373
+    if (!DebugCounter::shouldExecute(FwdCounter))
+      continue;
+
----------------
Add a debug print saying we're skipping because of debug counter.


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


https://reviews.llvm.org/D41835