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

[Tim Shen via Phabricator via llvm-commits]

timshen 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);
   }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D6594