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

[Chandler Carruth]

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.