[patch] Check that we don't try to RAUW a value with a ConstantExpr that uses Value.

[Duncan P. N. Exon Smith]

On 2014-May-12, at 16:06, Rafael Espíndola <rafael.espindola at gmail.com> wrote:

>> I wonder if you can make this cheaper, though.  Am I right that all
>> sub-exprs of a `ConstantExpr` must also be `ConstantExpr`s?  If so:
> 
> No, at the leaves we get other types of constants (like GlobalValues).
> The test being added is an example, it is a GEP of a GlobalValue.

Heh... right.

> What can be done is to make the Cache contain only the ConstantExprs.
> What do you think of the attached patch?

I still think it unnecessarily penalizes the case where `V` is not a
`Constant`.

LGTM with the following change:

> +static bool contains(Value *Expr, Value *V) {

    if (Expr == V)
      return true;
    if (!isa<Constant>(V))
      return false;

> +  SmallPtrSet<ConstantExpr *, 4> Cache;
> +  return contains(Cache, Expr, V);
> +}

If you agree, then there's a cheaper place for the equality comparison
in the recursive function:

    static bool contains(SmallPtrSet<ConstantExpr *, 4> &Cache, Value *Expr,
                         Constant *V) {
      auto *CE = dyn_cast<ConstantExpr>(Expr);
      if (!CE)
        return false;

      if (!Cache.insert(CE))
        return false;

      for (Value *O : CE->operands()) {
        if (O == V || contains(Cache, O, V))
          return true;
      }
      return false;
    }

    static bool contains(Value *Expr, Value *V) {
      if (Expr == V)
        return true;

      Constant *CV = dyn_cast<Constant>(V);
      if (!CV)
        return false;

      SmallPtrSet<ConstantExpr *, 4> Cache;
      return contains(Cache, Expr, CV);
    }

Since the recursive function can't be inlined, I'd be tempted to
duplicate the `dyn_cast<ConstantExpr>` as well (one copy in the outer
function, the other inside the `for` loop).  All this saves is a
function call, though, so I'm not sure whether it's worth the extra
complexity.