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

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