[PATCH] D59788: [MemDepAnalysis] Allow caller to pass in an OrderedBasicBlock.

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

I was working on restarting the in-instruction order tracking discussion, but the pathological test case I came up with didn't benefit from it:

  const std::vector<unsigned> &lookup(std::string k) {
    static std::map<std::string, std::vector<unsigned>> the_map = {
      {"s", {1, 2, 3}},
      {"s", {1, 2, 3}},
      ... repeat 200x, or arbitrarily long
    };
    return the_map[k];
  }

This causes clang to generate a very large BB that DSE spends a lot of time on. However, invalidating the order numbers only on removal wasn't enough to drastically improve performance, so I lost the motivation to drive it through. The ordering helped significantly with my less simplified test case (`std::map<unsigned, std::vector<StringRef>>`, see rC345329 <https://reviews.llvm.org/rC345329> which removed it), but I wasn't happy with it.



================
Comment at: llvm/include/llvm/Analysis/MemoryDependenceAnalysis.h:456-457
+      const MemoryLocation &MemLoc, bool isLoad, BasicBlock::iterator ScanIt,
+      BasicBlock *BB, Instruction *QueryInst, unsigned *Limit = nullptr,
+      OrderedBasicBlock *OBB = nullptr);
 
----------------
This function only has one real call site, which is in `getPointerDependencyFrom`. Can you make these parameters non-optional?


================
Comment at: llvm/lib/Analysis/MemoryDependenceAnalysis.cpp:446-450
   if (!Limit) {
     unsigned DefaultLimit = BlockScanLimit;
     return getSimplePointerDependencyFrom(MemLoc, isLoad, ScanIt, BB, QueryInst,
-                                          &DefaultLimit);
+                                          &DefaultLimit, OBB);
   }
----------------
I've seen this pattern across LLVM, and I've never particularly liked it. I don't think it's as easy to optimize as the author probably thought it was. I think this would be simpler:
```
unsigned DefaultLimit = BlockScanLimit;
if (!Limit)
  Limit = &DefaultLimit;
...
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59788/new/

https://reviews.llvm.org/D59788