[PATCH] D31583: StackColoring: smarter check for slot overlap

[Than McIntosh via Phabricator via llvm-commits]

thanm added a comment.

Hi Ariel,

I am thinking that maybe there is something wrong with the patch on phab-- I downloaded it and it would not compile:

  StackColoring.cpp:1050:5: error: use of undeclared identifier 'UI'
      UI->getNextValue(Indexes->getZeroIndex(), VNInfoAllocator);

I fixed that up in the obvious way.

Aside from that, I'd say that so far I don't think I completely understand how your strategy works. Part of this may be due simple to terminology and the names you've given to the new things.

For example, I don't get why you've chosen the name "Use" for the new set in BlockInfo. The fact that gets recorded in this set is not really a "use" of the stack slot. Consider these two blocks:

  BB1:
    lifetime_end slot 5
    branch
  
  BB2:
    lifetime_end slot 5
    lifetime_start slot 5
    use of slot 5
    branch

The way the code works now, "Use" will be set for BB1 and not for BB2, even though BB2 has an actual use (reference to) the slot and BB2 does not. This is very confusing for people reading the code.

Similar confusion over your use of the term "active" in the comment ("active" is also used at various other places in the file comments, and I don't think your definition of "active" is the same as that used in the other places). Ditto for "IntervalStarts" -- why is it named this way?

Another point of confusion is over the use of the term "interference graph" in the comments-- the data structure being used to capture interferences is not really a graph, there are no explicit edges to speak of. A real interference graph would be an improvement over what's there (could be more precise in a number of cases), but that's not what exists in the code today.

A final question is what sort of testing you've done so far to make sure this works for all the odd corner cases.  LNT and bootstrap build are two things that come to mind.

With that said, please keep working on this, I think it's worth pursuing.  I am OOO all this week and possibly next, so I won't have a lot of time to look at it, but I'll get there eventually.

Regards,

Than


https://reviews.llvm.org/D31583