[PATCH] D6594: Fix for bug 8281 - Extremely slow assembling and disassembling of ptrtoint

[Chenguang Wang via Phabricator via llvm-commits]

wecing marked 3 inline comments as done.
wecing added inline comments.


================
Comment at: llvm/lib/IR/ConstantFold.cpp:365
+    Constant *E = getFoldedSizeOf(ATy->getElementType(), DestTy, true, Cache);
+    return Cache[Ty] = ConstantExpr::getNUWMul(E, N);
   }
----------------
timshen wrote:
> wecing wrote:
> > timshen wrote:
> > > timshen wrote:
> > > > This is correct today, but newly added `return` statements in the future might miss the `Cache[Ty] = ` part.
> > > > 
> > > > Do you mind doing the following:
> > > > ```
> > > > static Constant *getFoldedSizeOfImpl(Type *Ty, Type *DestTy, bool Folded,
> > > >                                  DenseMap<Type *, Constant *> &Cache) {
> > > >   // The actual implementation, no explicit caching.
> > > >   // It recursively calls into getFoldedSizeOf below.
> > > > }
> > > > 
> > > > static Constant *getFoldedSizeOf(Type *Ty, Type *DestTy, bool Folded,
> > > >                                  DenseMap<Type *, Constant *> &Cache) {
> > > >   Constant*& V = Cache[Ty];
> > > >   if (V == nullptr)
> > > >     V = getFoldedSizeOfImpl(...);
> > > >   return V;
> > > > }
> > > > ```
> > > ```
> > >   Constant*& V = Cache[Ty];
> > >   if (V == nullptr)
> > > ```
> > > 
> > > Actually we should use `result = Cache.insert(...); if (result.second) ...` instead, since `nullptr` might be a valid cached value.
> > Thank you @timshen ! I think you made a good point, but your proposal introduces another problem: now within `getFoldedSizeOfImpl()`, people must be very careful to always call `getFoldedSizeOf()` instead of `getFoldedSizeOfImpl()` to get the caching behavior, which is also pretty error-prone.
> > 
> > A better solution might be to create a wrapper type for the `Constant *`, so `getFoldedSizeOf()` would be something like:
> > 
> > ```
> > #define CACHE_VALUE(v) (new cached_val<Constant *>(Cache[Ty] = v))
> > 
> > static cached_val<Constant *> getFoldedSizeOf(..., DenseMap<...> *Cache) {
> >   ...
> >   if (ArrayType *ATy = dyn_cast<ArrayType>(Ty)) {
> >     Constant *N = ConstantInt::get(DestTy, ATy->getNumElements());
> >     cached_val<Constant *>E = getFoldedSizeOf(ATy->getElementType(), DestTy, true, Cache);
> >     return CACHE_VALUE(ConstantExpr::getNUWMul(E.value, N));
> >   }
> >   ...
> > }
> > 
> > static Constant *getFoldedSizeOf(...) {
> >   return getFoldedSizeOf(...).value;
> > }
> > ```
> > 
> > But that is obviously an overkill for the problem, so I would rather keep it simple here. WDYT?
> Yes, I agree that using cached_value would be an overkill. I would say adding a comment about the recursive call target is sufficient.
I see. Done, PTAL.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D6594