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

[Hans Wennborg]

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?

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

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

 - 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>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lvi.patch
Type: text/x-patch
Size: 7037 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141121/2c860fad/attachment.bin>