[PATCH] D18586: Allow DeadStoreElimination to track combinations of partial later wrties

[Chandler Carruth via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.

In http://reviews.llvm.org/D18586#451855, @hfinkel wrote:

> Rebased, and replaced the use of IntervalMap with std::map. The reviewers are certainly right: we gain little by using an IntervalMap here (because IntervalMap does not handle overlapping inserts, so we end up doing the merging ourselves anyway).
>
> In addition to the reviews here, I also discussed this with folks offline, and the conclusion was to go with a map or some general data structure at first. If we see a compile-time hit from this related to small stores, we can always add code to use a bitmap for small stores.


I think I'm OK with this for now. I like the idea of using a simpler data structure at first. But please add a comment to the data structure that explains both the semantics (especially the surprising {end, start} representation, I had to read a fair amount of code to even see this) and captures a lot of the discussion we've had about tradeoffs around different data structures in case you or I aren't the ones to try to improve this later.

Everything below is further thoughts that might be distilled into comments, not anything I'm asking for right now:

My suspicion is that a sorted vector would work better than a map here. Among other things, it is very easy to have the intervals be represented in a natural way within the vector, and merely use a custom comparison to achieve the sorting behavior you want. Additionally, the overhead for the std::map will be roughly the same as the value size which is always an unfortunate tradeoff.

But as I said I'm happy for you to look at this as a follow-up patch. It shouldn't block the review as the code will be largely the same at this point, and the current version is almost certainly the simplest and most basic approach so it seems a good starting position.

>From the discussion, I suspect the eventual end state will be ta use a BitVector for "small" regions, and a sorted vector for "large" regions. But that will definitely be quite complex and so I wouldn't want to go there until we have some useful test cases that show serious compile time hits here.

Thanks again,
-Chandler


http://reviews.llvm.org/D18586