[PATCH] SubRegister liveness tracking

Renato Golin renato.golin at linaro.org
Tue Oct 8 07:30:39 PDT 2013


First comments about 0001:

CopyFrom seems to assume the dest range is empty, or that there isn't any
duplicates between the two (this and other), is that assumption safe on all
accounts? If it is, shouldn't there be an assert? If not, shouldn't there
be a collision detection?

The SingleLinkedListIterator<> seems a lot of boilerplate on the wrong
place. Isn't there another container in LLVM that can do that? If not,
shouldn't this code be in a more general header?

You have good comments, but they're not consistent. Some have \brief,
others don't. Also, not all new functions have comments. While some
functions are obvious, it serves the purpose of better Doxygen pages.

+void LiveInterval::RemoveEmptySubRanges() {
+  while (SubRanges != 0 && SubRanges->empty())
+    SubRanges = SubRanges->Next;

This looks like an over-optimization of special cases, that would probably
be dealt with by the second loop, which is also over-complicated. If all
you need is to jump the Next pointer when the SubRange is NULL, you can do
that in a single loop, with less confusion, and less premature optimization.

For my education, SubRange::LaneMask is one bit per lane, right? So,
0x00000011 is the same as D0 or S0+S1, right?

The crucial function verifyLiveInterval() et al. seem ok, though I confess
I haven't checked for all possible corner cases. I suspect you'll have
tests for all of them.

cheers,
--renato


On 8 October 2013 15:00, Renato Golin <renato.golin at linaro.org> wrote:

> Hi Mathias,
>
> I'm not sure how's the best way to review your patches. There are too
> many, and they're all related...
>
> Would that be better on Phabricator? GitHub had a good way to review a
> series of patches on the same pull request, not sure Phabricator does the
> same thing.
>
> I'm looking at them in order, but the emails will soon entangle when we
> start reviewing comments from different patches...
>
> cheers,
> --renato
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131008/f85b1536/attachment.html>


More information about the llvm-commits mailing list