[cfe-commits] r153297 - /cfe/trunk/lib/Analysis/CFG.cpp

[Argyrios Kyrtzidis]

On Mar 24, 2012, at 11:36 PM, NAKAMURA Takumi wrote:

> 2012/3/23 Argyrios Kyrtzidis <akyrtzi at gmail.com>:
>> Author: akirtzidis
>> Date: Thu Mar 22 19:59:17 2012
>> New Revision: 153297
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=153297&view=rev
>> Log:
>> [CFG] Cache boolean evaluations of expressions to avoid multiple re-evaluations
>> during construction of branches for chained logical operators.
>> 
>> This makes -fsyntax-only for test/Sema/many-logical-ops.c about 32x times faster.
>> 
>> With measuring SemaExpr.cpp I see differences below the noise level.
>> 
>> Modified:
>>    cfe/trunk/lib/Analysis/CFG.cpp
>> 
>> Modified: cfe/trunk/lib/Analysis/CFG.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/CFG.cpp?rev=153297&r1=153296&r2=153297&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Analysis/CFG.cpp (original)
>> +++ cfe/trunk/lib/Analysis/CFG.cpp Thu Mar 22 19:59:17 2012
>> @@ -286,6 +286,10 @@
>>   CFG::BuildOptions::ForcedBlkExprs::value_type *cachedEntry;
>>   const Stmt *lastLookup;
>> 
>> +  // Caches boolean evaluations of expressions to avoid multiple re-evaluations
>> +  // during construction of branches for chained logical operators.
>> +  llvm::DenseMap<Expr *, TryResult> CachedBoolEvals;
>> +
>>  public:
>>   explicit CFGBuilder(ASTContext *astContext,
>>                       const CFG::BuildOptions &buildOpts)
>> @@ -439,12 +443,65 @@
>>   /// tryEvaluateBool - Try and evaluate the Stmt and return 0 or 1
>>   /// if we can evaluate to a known value, otherwise return -1.
>>   TryResult tryEvaluateBool(Expr *S) {
>> -    bool Result;
>>     if (!BuildOpts.PruneTriviallyFalseEdges ||
>> -        S->isTypeDependent() || S->isValueDependent() ||
>> -        !S->EvaluateAsBooleanCondition(Result, *Context))
>> +        S->isTypeDependent() || S->isValueDependent())
>>       return TryResult();
>> -    return Result;
>> +
>> +    if (BinaryOperator *Bop = dyn_cast<BinaryOperator>(S)) {
>> +      if (Bop->isLogicalOp()) {
>> +        // Check the cache first.
>> +        typedef llvm::DenseMap<Expr *, TryResult>::iterator eval_iterator;
>> +        eval_iterator I;
>> +        bool Inserted;
>> +        llvm::tie(I, Inserted) =
>> +            CachedBoolEvals.insert(std::make_pair(S, TryResult()));
> 
> Why to insert a placeholder with <S, TryResult()> at first?

It is more efficient when inserting since it does one lookup instead of 2 lookups (one find and one insert).
But it affected correctness, thank you for fixing it!

-Argyrios

> Rewritten in r153407. Lemme know if early insertion would be needed here anyway.
> 
>> +        if (!Inserted)
>> +          return I->second; // already in map;
>> +
>> +        return (I->second = evaluateAsBooleanConditionNoCache(S));
> 
> The iterator *I* would be invalidated after
> evaluateAsBooleanConditionNoCache(S).
> It caused memory leak at compiling ARMAsmParser.cpp.
> Should be fixed in r153406.
> 
> ...Takumi