[PATCH] SubRegister liveness tracking

Renato Golin renato.golin at linaro.org
Wed Oct 9 09:10:01 PDT 2013


Patches 0012 ~ 0018 look ok. As usual, small comment on new functions.

Specific comments follow...

On 0012:

+      // TODO: think if this can actually happen...
+      if (*M == ~0u)
+        PrintFatalError("Invalid regunitlanemask");

that could be an assert...

On 0014, assign/unassign/checkRegUnitInterference have very similar
structures, but I can't think of a way to common up that code without
introducing virtual dispatch or function pointers. :( Maybe it's ok the way
it is.

On 0015:

+    if (Mask != MaxMask) {
+      // No need for further tracking if complete subranges are missing.
+      CancelAllKills = true;
+    }

shouldn't this be (Mask == MaxMask)? As in: all masks join up to the total
mask, thus, cancel all kills.

On 0016:

+        // when tracking subregister liveness the main range must start new
+        // values on partial register writes, even if there is no read.
+        if (!MRI->tracksSubRegLiveness() || LaneMask != 0 ||
!hasSubRegDef) {

For my education, you're making sure there isn't a write on a sub-register,
via a super-register's lane. Ie., in this case, the main range must create
a write to the specific sub-register, so you can keep score of its
liveness, right?

On 0018:

I'm thinking we could add all 17 previous patches, let the buildbots run
and stabilize (if they get red), then, when everything is fine, enable them
by default on ARM. That'll give us at least two stages in which to fix
problems.

If you're feeling adventurous, you could separate your patch into
functional groups and apply them a group at a time, and see if there's any
bots or additional tests breaking, but I personally don't think that's
necessary.

A general comment about all patches, thank you for working on this. The
code is clear and you have added a lot of comments in between the lines,
making my life a lot easier to review, and helped me learn about how the
live ranges are computed, which will help other people read the code in the
future, too.

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


More information about the llvm-commits mailing list