[PATCH] D12588: Introduce target hook for optimizing register copies

[Quentin Colombet via llvm-commits]

qcolombet added a comment.

Hi Matt,

I would have expected shareSameRegisterFile to be a win for every target, but you seem to drop that check.
Is that intentional?

I am mostly fine with the change, just not sure about what to do with shareSameRegisterFile. See my inline comment for further discussions.

Thanks,
-Quentin


================
Comment at: include/llvm/Target/TargetRegisterInfo.h:512
@@ +511,3 @@
+                             const TargetRegisterClass *SrcRC,
+                             unsigned SrcSubReg) const;
+
----------------
Does it make sense to expose this API here?
I.e., do we expect more users of this?
If not, I would rather keep it where it was as a static function or at least being private.

================
Comment at: include/llvm/Target/TargetRegisterInfo.h:523
@@ +522,3 @@
+    // If this source does not incur a cross register bank copy, use it.
+    return shareSameRegisterFile(DefRC, DefSubReg, SrcRC, SrcSubReg);
+  }
----------------
Just return false, see further for the complete suggestion.

================
Comment at: lib/CodeGen/PeepholeOptimizer.cpp:667
@@ -696,5 +666,3 @@
       const TargetRegisterClass *SrcRC = MRI->getRegClass(CurSrcPair.Reg);
-
-      // If this source does not incur a cross register bank copy, use it.
-      ShouldRewrite = shareSameRegisterFile(*TRI, DefRC, SubReg, SrcRC,
-                                           CurSrcPair.SubReg);
+      ShouldRewrite = TRI->shouldRewriteCopySrc(DefRC, SubReg, SrcRC,
+                                                CurSrcPair.SubReg);
----------------
shareSameRegisterFile is supposed to produce register coalescer friendly copies and should benefit every target.
So I would have expect this check to be
shareSameRegisterFile || TRI->shouldRewriteCopySrc.

I may be wrong, but I want to be sure the change is intentional.


http://reviews.llvm.org/D12588