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

[Pete Cooper]

> On Nov 21, 2014, at 5:55 PM, Hans Wennborg <hans at chromium.org> wrote:
> 
> On Fri, Nov 21, 2014 at 4:09 PM, Pete Cooper <peter_cooper at apple.com> wrote:
>> So, i’ve worked through this in lldb and ultimately the wrong decision is about the value of %inc2 in %for.cond.
>> 
>> When we process '%i.07 = phi i32 [ 1, %for.body.lr.ph ], [ %inc2, %for.cond ]’, we get 1 from the first value, i.e., a constantrange [1,2)
>> We then ask for the result for %inc2 from %for.cond.  As you pointed out, its in the cache so we return its Result (which is still undefined).  We then (in solveBlockValuePHINode), do 'Result.mergeIn(EdgeResult)’ which ignores the undefined RHS and we think the result from our PHI is the constantrange.
>> 
>> So, there’s a couple of things going wrong here.  Firstly, i’m worried that ‘Result.mergeIn’ returned false, but we didn’t check the return value in solveBlockValuePHINode.  I think this is a bug regardless of my patch, although I have no test case.
> 
> mergeIn() returns whether the value changed. Do we need to check that here?
Having looked in to it more, i’m not sure if we do.  If we merge a constant in to say and undefined then it will replace the undefined and return true.  If we merge an undefined in to a constant it will leave it a constant and return false.  So the return value doesn’t seem to be telling us much about the actual values involved.
> 
>> The other issue, which I think is a result of my patch, is that getEdgeValue should probably return false if the value is undefined as that means we still need to process that edge.  I’m actually getting more and more tempted to stop using undefined in my patch and make an explicit enum value which means ‘processing predecessors’ or at least ‘unknown’ so we know its value is TBD.
> 
> Hmm, I guess Undefined is never a valid result after successfully
> processing an edge? I wonder why we're not just returning that instead
> of true/false everywhere.
Thats true.  I’m using undefined in the latest patch I resent to mean that a value is cached, but not yet defined.  If we call getBlockValue and it returns undefined then the value is now on its second try with the given BB and we should stop there before adding it again to the map.

So I think i’m using undefined being returned from the cache in the same way you are using a set of the seen BB/Value pairs.  I’m happy with either approach in principle to be honest, although yours is cleaner.
> 
>> What do you think?
> 
> I think if we're returning false in solveBlockValue() with the
> intention of returning to it, we shouldn't put a value in the cache.
> If we do, it can get picked up by others, and changing it later seems
> perilous.
> 
> So we should take care not to touch the cache unless we're actually
> done, just return false, and continue with the stack until we return.
> To break cycles, we have to make sure never to push the same
> block-value pair on the stack if it's already there. If we try to do
> that, we make the value Overdefined.
> 
> Attaching one patch idea. I haven't benchmarked the impact of the
> extra std::set lookup (which could maybe be a DenseSet) on compile
> time, but I verified that this recovers the binary size for Chromium,
> and passes yours and Jiangning's tests.
I like the patch.  Given that ODCacheUpdater is only ever used in the places you’ve updated, i think we can just hide the majority of what you’ve done its its implementation.  So lines such as these will actually be unchanged, and the code inside markResult could hopefully be changed instead.

-    return ODCacheUpdater.markResult(solveBlockValueNonLocal(BBLV, Val, BB));
+    if (!solveBlockValueNonLocal(Res, Val, BB))
+      return false;
+    Cache[BB] = Res;
+    return ODCacheUpdater.markResult(true);

Thanks,
Pete
> 
> - Hans
> 
> 
>>> On Nov 21, 2014, at 3:34 PM, Pete Cooper <peter_cooper at apple.com> wrote:
>>> 
>>> 
>>>> On Nov 21, 2014, at 3:30 PM, Hans Wennborg <hans at chromium.org> wrote:
>>>> 
>>>> On Fri, Nov 21, 2014 at 3:13 PM, Pete Cooper <peter_cooper at apple.com> wrote:
>>>>> Thanks Hans!  I was just trying to reduce it.
>>>>> 
>>>>> I’ll take a look now.  Probably something i’ve done is breaking when there’s loops.
>>>> 
>>>> I think the problem is that when we're resetting BBLV to Undefined,
>>>> that value goes in the cache, hasBlockValue() is now returning true
>>>> for that value, and others might pick it up - and Undefined is the
>>>> wrong value.
>>> Ah, I didn’t know undefined really meant anything to this algorithm.  I figured it would mean unknown, but if it actually means undef and that the optimizer can choose a value then i can see there being a problem.
>>>> 
>>>> What we'd really want to do is to remove BBLV from the cache until
>>>> we're done computing its value, but then we're not terminating cycles
>>>> anymore. Hmm.
>>> I think we’ll still terminate cycles because we’ll always eventually hit a loop pre-header or the start of the function, but i haven’t thought about it enough to say what the complexity would be in that case.
>>>> 
>>>> 
>>>>>> On Nov 21, 2014, at 3:12 PM, Hans Wennborg <hans at chromium.org> wrote:
>>>>>> 
>>>>>> On Thu, Nov 20, 2014 at 7:51 PM, Pete Cooper <peter_cooper at apple.com> wrote:
>>>>>>> But something is going wrong. I'm attaching a reduction of the problem
>>>>>>> I saw. With your patch applied, using "clang -O2 /tmp/a.cc", the
>>>>>>> attached program goes into an infinite loop.
>>>>>>> 
>>>>>>> Thanks!  I’ll take a look.
>>>>>> 
>>>>>> I've reduced it (at least a little..) into an IR test (attached). With
>>>>>> your patch applied, run through opt like so:
>>>>>> 
>>>>>> $ opt -correlated-propagation /tmp/reduced.ll -S
>>>>>> 
>>>>>> Note how the definition of %i.1 changes from
>>>>>> 
>>>>>> %i.1 = phi i32 [ %add, %for.body ], [ %i.07, %if.then ]
>>>>>> 
>>>>>> to
>>>>>> 
>>>>>> %i.1 = phi i32 [ 2, %for.body ], [ 1, %if.then ]
>>>>>> 
>>>>>> which is not correct.
>>>>>> 
>>>>>> - Hans
>>>>>> <reduced.ll>
> <lvi.patch>