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

[Chenguang Wang via Phabricator via llvm-commits]

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:
> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D6594