[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:58 PM, Hans Wennborg <hans at chromium.org> wrote:
> 
> On Fri, Nov 21, 2014 at 5:36 PM, Pete Cooper <peter_cooper at apple.com> wrote:
>> Attached a new diff.
> 
> Can you try sending it again? It doesn't open in my editor for some reason.
Weird.  Sorry about that.  Here it is again.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: lvi.diff
Type: application/octet-stream
Size: 6272 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141121/8d5b43a4/attachment.obj>
-------------- next part --------------

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