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

[Chandler Carruth via llvm-commits]

chandlerc added a comment.

Minor comments below about the data structure, but I really wonder whether IntervalMap is the right tool for the job here. It seems awfully heavyweight, although it is clearly very efficient... And it seems to be fighting you on several fronts:

1. You want a set, not a map.
2. You can't insert overlapping ranges.
3. The sparseness isn't likely to be useful as you're mostly interested in fairly small memory regions.

What about using a DenseMap<Instruction *, BitVector> and setting one bit per byte? My thoughts:

- No complexity around overlaps.
- No extra allocation for 64 byte and smaller regions. Pretty minimal allocation even for larger regions, up to 512 bytes no problem.
- Super simple implementation, probably much faster for small regions to just set bits and test them.

The major downside I see is that it doesn't scale gracefully to *large* memory regions like a memset might hit...

What do you think? Maybe set an upper bound on size (512 bytes seems a good bound, keeps the BitVector on a cache line) and use BitVector? Is it worth the complexity of using a BitVector when small and an IntervalMap when large?


================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:348-349
@@ -337,1 +347,4 @@
 
+typedef IntervalMap<int64_t, std::tuple<>, 4,
+                    IntervalMapHalfOpenInfo<int64_t>> OverlapIntervalsTy;
+typedef DenseMap<Instruction *, OverlapIntervalsTy> InstOverlapIntervalsTy;
----------------
Ugh, so you really want an interval *set*. Is it too hard to build one?

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:444-448
@@ +443,7 @@
+      int64_t(LaterOff + Later.Size) >= EarlierOff) {
+    if (!IOL.count(DepWrite))
+      IOL.insert(std::make_pair(DepWrite, OverlapIntervalsTy(OLAlloc)));
+
+    // Insert our part of the overlap into the map.
+    auto &IM = IOL.find(DepWrite)->second;
+    DEBUG(dbgs() << "DSE: Partial overwrite: Earlier [" << EarlierOff << ", " <<
----------------
Find first, and assign the iterator on insert? It'd be particularly nice to add a try_emplace to DenseMap so that you can do this with a single lookup. =/


http://reviews.llvm.org/D18586