[PATCH] D30350: [LSR] Add a cap for reassociation of AllFixupsOutsideLoop type LSRUse to protect compile time

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.

I think the code can be structured to make things more robust against future changes.  PTAL and see if you agree:



================
Comment at: lib/Analysis/ScalarEvolution.cpp:1520
+
+  // use by-copy capture for Ty because we want to use its value at the func
+  // start.
----------------
s/use/Use/
s/func/function/


================
Comment at: lib/Analysis/ScalarEvolution.cpp:1547
   void *IP = nullptr;
-  if (const SCEV *S = UniqueSCEVs.FindNodeOrInsertPos(ID, IP)) return S;
+  // Before doing any expensive analysis, check to see if we've already
+  // computed a SCEV for this Op and Ty.
----------------
No need to move this comment?


================
Comment at: lib/Analysis/ScalarEvolution.cpp:1734
+    const SCEV *Op, Type *Ty,
+    SmallDenseMap<std::pair<const SCEV *, Type *>, const SCEV *, 8> &Cache) {
+  auto It = Cache.find({Op, Ty});
----------------
Can you please make `SmallDenseMap<std::pair<const SCEV *, Type *>, const SCEV *, 8>` a typedef?


================
Comment at: lib/Analysis/ScalarEvolution.cpp:1739
+
+  // use by-copy capture for Ty because we want to use its value at the func
+  // start.
----------------
s/use/Use/
s/func/function/

Also `CacheResult` may be a better name (sorry for not suggesting it before! -- hopefully this should be a quick find-replace).

Also, what do you think about an RAII object (under `#ifndef NDEBUG`) that checks that `{Op, Ty}` is in the cache on destruction?  I can imagine someone adding a new return path and forgetting to call `CacheResult`.



Actually, a better solution would be to make the caching guarantee true by construction:

`getZeroExtendExpr` creates a cache, and calls `getZeroExtendExprCached` with the cache.  `getZeroExtendExprCached` calls `getZeroExtendExprImpl` to do the heavy lifting (which recursively calls `getZeroExtendExprCached` when needed), and also does the pre-check in the cache, and also the insertion into the cache (that way `getZeroExtendExprImpl` does not have to remember to insert into the cache at each return site).


Repository:
  rL LLVM

https://reviews.llvm.org/D30350