[llvm-commits] [PATCH][SOPT] Fix assertion in Reassociate

[Shuxin Yang]

On 11/7/12 1:26 AM, Chandler Carruth wrote:
> On Tue, Nov 6, 2012 at 5:14 PM, Shuxin Yang <shuxin.llvm at gmail.com> wrote:
>>> Two quick questions:
>>> 1. Is it generic enough to expose the class as a LLVM ADT?
>> Actually I used template to implement this worklist at beginning, hoping it
>> will be promoted into ADT.
> I think there are some real problems with the new worklist
> implementation that would also prevent it from going directly into the
> ADT, but honestly, we should probably fix those and get a proper
> FIFO-optimized worklist into the ADT if that's an important construct.
> Specifically, use of std::set seems really unfortunate here, and use
> of std::deque seems really bad. deque has pretty terrible performance
> overheads on a lot of STL implementations in the wild.
>
>
> But I'd really like to understand exactly why SetVector didn't work.
> There are several places in LLVM that use it as a FIFO construct:
>
> // Note that the 'size' method is called on each trip as the worklist may grow.
> for (unsigned Idx = 0; Idx != Worklist.size(); ++Idx) {
>    Instruction *I = Worklist[Idx];
>    // ...
> }
>
> The only requirement here is that we can keep the Instruction* (or
> value handles) alive until the entire worklist is destroyed, and that
> they will continue to be correct in the context of of the set
> semantics.
>
> These are usually both achievable by (at most) keeping a "kill list"
> of instructions to be deleted, and after finishing the processing of
> the worklist, walking the kill list in whatever order deleting the
> instructions. We don't get into ABA problems with the pointer-based
> set equality, etc.
>
>
> If AssertingVH makes this hard, drop it. AssertingVH provides no
> intrinsic value other than asserts in certain circumstances, and
> really that value is dubious. I've consistently regretted using it in
> conjunction with worklists and other data structures of the sort.
We need to physically delete the instruction in order to reduce the # of 
reference.
Reassociates differenciate hasOneUse() or not.

We cannot modify the element via iterator. We have to call ::remove to 
physically erase the dead element, that we be really bad.