[PATCH] [PeepholeOptimizer] Refactor the advance copy optimization to take advantage of the isRegSequence property

[Quentin Colombet]

Hi Hal,

Thanks for your detailed feedbacks!

I’ll update the patch accordingly.

Cheers,
-Quentin

================
Comment at: lib/CodeGen/PeepholeOptimizer.cpp:238
@@ +237,3 @@
+    /// If \p Reg is a physical register, a value tracker constructed with
+    /// this constructor will not find any alternative source.
+    ValueTracker(unsigned Reg, unsigned DefSubReg,
----------------
hfinkel at anl.gov wrote:
> I would really like a more-verbose comment here (maybe with an example) to explain what this means. And, perhaps, why you might want to find an alternative source for a physical register or not.
Sure, I’ll add a comment about that.
Technically, you cannot find a new source for a physical register with that interface because you do not know what instruction defines it.
The next constructor provides an interface to overcome that.

Regarding finding an alternative source for a physical register, this might make sense to do that to be able to get rid of a target specific instruction and thus, be able to take advantage of the register coalescer.

================
Comment at: lib/CodeGen/PeepholeOptimizer.cpp:598
@@ +597,3 @@
+  /// Each call of this method moves the current source to the next
+  /// rewritable source.
+  /// \return True if a rewritable source has been found, false otherwise.
----------------
hfinkel at anl.gov wrote:
> I don't understand this comment (and/or the associated logic below). When this is called we set CurrentSrcIdx = 1, and then we exit early if CurrentSrcIdx == 1. A comment on how this all works would be nice.
Nice catch, the comment is indeed very obscure.
I will update that.

The idea is the following.

Let CopyLike be the instruction being rewritten, where CopyLike has one definition and one source.
In other words, we have:
dst = CopyLike src

If CurrentSrcIdx != 0 (i.e., if CurrentSrcIdx == 1), it means we are looking for the next source of src, which cannot be determined from CopyLike.
However, if we are looking at CurrentSrcIdx == 0, we want the next source of dst, which is src.
Therefore SrcReg == src and TrackReg == dst.

================
Comment at: lib/CodeGen/PeepholeOptimizer.cpp:811
@@ +810,3 @@
+/// Two register classes are considered to be compatible if they share
+/// the same register bank.
+/// New copies issued by this optimization are register allocator
----------------
hfinkel at anl.gov wrote:
> What code below enforces this "register bank" constraint?
PeepholeOptimizer::findNextSource.

================
Comment at: lib/CodeGen/PeepholeOptimizer.cpp:851
@@ +850,3 @@
+  // We do not do that for now, because the intermediate from is
+  // currently not supported and we would need to add some book keeping
+  // to revert the unsupported patterns if the clean-up is not possible.
----------------
hfinkel at anl.gov wrote:
> What exactly is not supported?
My bad, I thought "v0 = INSERT_SUBREG v1, v1.sub0, sub0” was incorrectly expanded during TwoAddressInstructionPass, same for REG_SEQUENCE v1.sub0, sub0, v1.sub1, sub1 when we compose sub-registers. I have double checked and in fact, this works.
This should be a TODO, driven by actually seeing this pattern.

http://reviews.llvm.org/D4874