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

Hal Finkel via llvm-commits llvm-commits at lists.llvm.org
Thu Mar 31 19:10:19 PDT 2016


----- Original Message -----
> From: "Chandler Carruth" <chandlerc at gmail.com>
> To: hfinkel at anl.gov, echristo at gmail.com, igor at azulsystems.com, mcrosier at codeaurora.org, dberlin at dberlin.org,
> eeckstein at apple.com
> Cc: jvanadrighem at gmail.com, junbuml at codeaurora.org, llvm-commits at lists.llvm.org
> Sent: Thursday, March 31, 2016 8:56:37 PM
> Subject: Re: [PATCH] D18586: Allow DeadStoreElimination to track combinations of partial later wrties
> 
> 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 if I used a SparseBitVector. Perhaps that's the best of both worlds?

 -Hal

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

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory


More information about the llvm-commits mailing list