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

[Pete Cooper]

Attached a new diff.

This checks the result of getBlockValue for undefined.  If we get an undefined value then that must be from a block we’re waiting on the information from.  If we try to get it again then we must have a cycle so at that point we break the cycle by moving to overdefined.

Thanks,
Pete

-------------- next part --------------
A non-text attachment was scrubbed...
Name: lvi.diff
Type: application/applefile
Size: 68 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141121/c9425d0a/attachment.bin>
-------------- next part --------------

> On Nov 21, 2014, at 5:04 PM, Hal Finkel <hfinkel at anl.gov> wrote:
> 
> [+Jiangning, Philip, Nick]
> 
> ----- Original Message -----
>> From: "Pete Cooper" <peter_cooper at apple.com>
>> To: "Hans Wennborg" <hans at chromium.org>
>> Cc: "llvm-commits at cs.uiuc.edu for LLVM" <llvm-commits at cs.uiuc.edu>
>> Sent: Friday, November 21, 2014 6:09:50 PM
>> Subject: Re: [llvm] r215343 - In LVI(Lazy Value Info),	originally value on a BB can only be caculated once,
>> 
>> 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
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
> 
> -- 
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory