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

[Ariel Ben-Yehuda via Phabricator via llvm-commits]

arielb1 added a comment.

In https://reviews.llvm.org/D31583#718444, @thanm wrote:

> 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.


Fixed that.

> 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.

BLI.Use refes, as it says, to a slot that begins and ends within the same block. Maybe I should have called it "BeginAndEnd" or something? Maybe I should have `Use` also include `Begin` (actually, let's do that)?

> 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?

I thought "active" was a new fresh term. I informally referred to it as "live", but unfortunately "live" has another meaning. Is there a better term than "active"?

> 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.

I was thinking about things theoretically.

> 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.

I bootstrapped rustc and passed all of its tests. I don't know how to bootstrap LLVM, so I didn't do that.

> 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.

Thanks.

> Regards,
> 
> Than




https://reviews.llvm.org/D31583