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

Rafael EspĂ­ndola rafael.espindola at gmail.com
Mon May 12 18:34:04 PDT 2014


> 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.


Committed as r208663.

Thanks,
Rafael



More information about the llvm-commits mailing list