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

[Pete Cooper]

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.

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.

What do you think?

Thanks,
Pete


> 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>
>>> 
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits