[PATCH] D84609: [MemDepAnalysis] Cut-off threshold reshuffling

[Evgeniy via Phabricator via llvm-commits]

ebrevnov added a comment.

I would suggest to separate an introduction of additional limit for total number of scanned instructions and reduction of the current limit for number of scanned blocks to a different change sets.

> Proposed action: no change, keep it at 100, which is ~80'th percentile - the cut-off is actively helpful.

80'th percentile doesn't look great to me. Once we introduce a total limit for scanned instructions I would suggest increasing it to at least 90't percentile. What do you think?



================
Comment at: llvm/include/llvm/Analysis/MemoryDependenceAnalysis.h:457
+                                        unsigned *LocalLimit = nullptr,
+                                        unsigned *GlobalLimit = nullptr);
 
----------------
I think we better avoid adding one more limit here since it complicates the API . It is enough to provide one limit to achieve the desired behavior. In particular a caller should specify a minimum of local and global limits.


================
Comment at: llvm/lib/Analysis/MemoryDependenceAnalysis.cpp:217
     // running time on extreme testcases.
-    --Limit;
-    if (!Limit)
+    --LocalLimit;
+    if (!LocalLimit)
----------------
I would suggest changing order of decrement and the check (as done in getSimplePointerDependencyFrom) to get more consistent behavior in case of zero default limit.


================
Comment at: llvm/lib/Analysis/MemoryDependenceAnalysis.cpp:390
+  unsigned DefaultLocalLimit = getDefaultBlockScanLimit();
+  unsigned DefaultGlobalLimit = getDefaultBlockScanLimit();
+  if (!LocalLimit)
----------------
I guess this should be retrieving total instruction limit.


================
Comment at: llvm/lib/Analysis/MemoryDependenceAnalysis.cpp:396
+
+  auto _ = make_scope_exit([&, Orig = *LocalLimit]() {
+    NumBlockInstructionsScannedMax.updateMax(Orig - *LocalLimit);
----------------
Minor note. It's recommended to avoid default capture modes. Would be better if you capture LocalLimit explicitly. Feel free to keep as is though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84609