[PATCH] D125335: Pass lamba to MachineCopyProp to determine which MI is treated as Copy instruction.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:97
+                                            bool UseCopyInstr) {
+  if (UseCopyInstr) {
+    return TII.isCopyInstr(MI);
----------------
Drop curly braces


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:101
+
+  if (MI.isCopy()) {
+    return Optional<DestSourcePair>(
----------------
Drop curly braces


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:144
         if (MachineInstr *MI = I->second.MI) {
-          RegsToInvalidate.insert(MI->getOperand(0).getReg().asMCReg());
-          RegsToInvalidate.insert(MI->getOperand(1).getReg().asMCReg());
+          assert(isCopyInstr(*MI, TII, UseCopyInstr) && "Expect copy");
+
----------------
Can you write this assert using `CopyOperands` instead of calling `isCopyInstr` twice?


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:185
+                 const TargetInstrInfo &TII, bool UseCopyInstr) {
+    assert(isCopyInstr(*MI, TII, UseCopyInstr) && "Tracking non-copy?");
 
----------------
Use `CopyOperands` in the assert instead of calling `isCopyInstr` twice


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:249
 };
-
 class MachineCopyPropagation : public MachineFunctionPass {
----------------
Put this blank line back


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:423
   // flags between Copy and PrevCopy because the value will be reused now.
-  assert(Copy.isCopy());
-  Register CopyDef = Copy.getOperand(0).getReg();
+  assert(isCopyInstr(Copy, *TII, UseCopyInstr));
+
----------------
Check `CopyOperands` instead of calling `isCopyInstr` twice


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:656
+
+      auto CopyOperands = isCopyInstr(MI, *TII, UseCopyInstr);
+      Register RegSrc = CopyOperands->Source->getReg();
----------------
Don't call `isCopyInstr` twice


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:822
       // Update matching debug values, if any.
-      assert(MaybeDead->isCopy());
-      Register SrcReg = MaybeDead->getOperand(1).getReg();
-      Register DestReg = MaybeDead->getOperand(0).getReg();
+      assert(isCopyInstr(*MaybeDead, *TII, UseCopyInstr));
+
----------------
This assert is useless. You already accessed the `Source` and `Destination` above, which would assert if the isCopyInstr had returned None.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:844
+                                       bool UseCopyInstr) {
+  assert(isCopyInstr(MI, TII, UseCopyInstr) && "MI is expected to be a COPY");
+
----------------
Don't call isCopyInstr twice


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:924
+    if (isCopyInstr(MI, *TII, UseCopyInstr)) {
+      auto CopyOperands = isCopyInstr(MI, *TII, UseCopyInstr);
+      Register DefReg = CopyOperands->Destination->getReg();
----------------
Don't call isCopyInstr twice


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125335/new/

https://reviews.llvm.org/D125335