[PATCH] SubRegister liveness tracking

[Renato Golin]

On 10 October 2013 03:13, Matthias Braun <mbraun at apple.com> wrote:

> You are right that patch was way more complicated that necessary. I
> rewrote most of it leaving a way simpler patch. Just needed a few
> invocation changes in the Coalescer patch.
>

0009 looks a lot better. I just have two more comments:

First you had:

-  SlotIndex MBBStart, MBBEnd;
-  tie(MBBStart, MBBEnd) = Indexes->getMBBRange(KillMBB);
+  SlotIndex MBBEnd = Indexes->getMBBEndIdx(KillMBB);

Then:

       // Check if VNI is live in to MBB.
+      SlotIndex MBBStart, MBBEnd;
       tie(MBBStart, MBBEnd) = Indexes->getMBBRange(MBB);

It looks like your second definition is shadowing the first. It seems it
was already, though. Or it might just be the way the patch is laid out, and
I'm not seeing the whole context.

Also, here:

+void LiveIntervals::pruneValue(LiveInterval &LI, SlotIndex Kill,
+                               SmallVectorImpl<SlotIndex> *EndPoints) {
+  pruneValue((LiveRange&)LI, Kill, EndPoints);

The naming issue I had still remains. I explain:

When looking for a function to prune a current value, I'll find two very
similar ones, with one that can be easily cast into another. If I use
auto-complete, or don't read the comments, I may pick the wrong one, and
it'll be hard for reviewers to spot this problem. If I do read the comments
and decide I would never have sub-ranges, I may pick the more restricted
one, and if the code changes in the future, and sub-ranges start being
valid (or I was wrong), it'll be hard to spot the difference.

What I'm getting at is that I think the one that iterates the sub ranges
should be public and the API we all should call, while the other is the
private implementation. Since you're casting the LI into a LR anyway, it'll
be just another level of indirection, and since the sub-range iterator will
be empty on irrelevant cases, it should be just a comparison and return.

Does that make sense?

cheers,
--renato
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131010/2d7f02bb/attachment.html>