[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