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

[Wei Mi via Phabricator via llvm-commits]

wmi added a comment.

Sanjoy, thanks for all the helpful comments.



================
Comment at: include/llvm/Analysis/ScalarEvolution.h:1163
+
+  typedef SmallDenseMap<std::pair<const SCEV *, Type *>, const SCEV *, 8>
+      CacheTypeForExtend;
----------------
sanjoy wrote:
> I think the usual naming scheme here is `ExtendCacheTy`?
Fixed.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:1522
+  const SCEV *ZExt = getZeroExtendExprImpl(Op, Ty, Cache);
+  Cache.insert({{Op, Ty}, ZExt});
+  return ZExt;
----------------
sanjoy wrote:
> If possible, you should
> 
> ```
> auto InsertResult = Cache.insert({{Op, Ty}, ZExt});
> assert(InsertResult.second);
> ```
> 
> otherwise state that it is possible (how?) that the key is already present in the cache.
I added the assertion and at least unittest and llvm testsuite didn't trigger it.


Repository:
  rL LLVM

https://reviews.llvm.org/D30350