[llvm] r215343 - In LVI(Lazy Value Info), originally value on a BB can only be caculated once,

[Pete Cooper]

> On Nov 20, 2014, at 4:27 PM, Hans Wennborg <hans at chromium.org> wrote:
> 
> On Thu, Nov 20, 2014 at 11:27 AM, Pete Cooper <peter_cooper at apple.com> wrote:
>> I came across this independently a few days ago.  My example was similar.  I
>> had
>> 
>> if (x == 0) {
>>  if (…) {  … } else { … }
>>  ++x
>> }
> 
> I'm still not very familiar with this analysis and its interaction
> with jump threading. Could you turn your example into a compilable
> function that I can experiment with?
I was looking at correlated-value-propagation actually, but I guess this would apply to jump threading too.

I’ll get you a testcase shortly, and a real patch.
> 
> 
>> The problem here is that as we walk from ++x up, we visit the BBs on the
>> inner if statement.  Their predecessors haven’t been processed yet so we
>> return out from solveBlockValueNonLocal which leaves them in the
>> ‘overdefined’ state.  We eventually get to ‘x == 0’ and start the walk back
>> down the CFG.  But on the walk back down we do the following check and
>> immediately bail.  So intermediate BBs get set to overdefined because of
>> unvisited predecessors and never leave that state.
>> 
>> if (!BBLV.isUndefined()) {
>>    DEBUG(dbgs() << "  reuse BB '" << BB->getName() << "' val=" << BBLV
>> <<'\n');
>> 
>> 
>> 
>>    // Since we're reusing a cached value here, we don't need to update the
>>    // OverDefinedCahce.  The cache will have been properly updated
>>    // whenever the cached value was inserted.
>>    ODCacheUpdater.markResult(false);
>>    return true;
>>  }
>> 
>> So, here’s the patch I have to fix the problem.  I believe it has no
>> algorithmic complexity issues as we do the same walk up the CFG from before,
>> but as we walk back down we will retry calling solveBlockValueNonLocal now
>> that all predecessors have a state.  I don’t have an IR test case this
>> minute, but I can get one tonight if needed.
>> 
>> @@ -672,8 +672,16 @@ bool
>> LazyValueInfoCache::solveBlockValueNonLocal(LVILatticeVal &BBLV,
>>       return true;
>>     }
>>   }
>> -  if (EdgesMissing)
>> +  if (EdgesMissing) {
>> +    // If we haven't yet processed all the predecessors, then put this
>> +    // value back to undefined so that we'll revisit it again.  Otherwise
>> it
>> +    // will be 'overdefined' once we return from here and we'll think that
>> +    // this is its final state, while in fact we don't know the state of
>> this
>> +    // value in this BB yet.
>> +    LVILatticeVal Result;
>> +    BBLV = Result;
>>     return false;
>> +  }
> 
> I applied your patch, but it seems to be causing miscompiles, as one
> of the programs got into an infinite loop and consumed all my memory.
Damn.  When I thought about the algorithm I convinced myself that it would terminate as you’ll always eventually chase the BBs up to a loop preheader or the function entry BB.  Perhaps I got that wrong.

Pete
> 
> - Hans