[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 6:25 PM, Pete Cooper <peter_cooper at apple.com> wrote:
>
> On Nov 21, 2014, at 6:20 PM, Hans Wennborg <hans at chromium.org> wrote:
>
> On Fri, Nov 21, 2014 at 6:15 PM, Pete Cooper <peter_cooper at apple.com> wrote:
>
>
> On Nov 21, 2014, at 5:55 PM, Hans Wennborg <hans at chromium.org> wrote:
>
> 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?
>
> Having looked in to it more, i’m not sure if we do.  If we merge a constant
> in to say and undefined then it will replace the undefined and return true.
> If we merge an undefined in to a constant it will leave it a constant and
> return false.  So the return value doesn’t seem to be telling us much about
> the actual values involved.
>
>
> 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.
>
> Thats true.  I’m using undefined in the latest patch I resent to mean that a
> value is cached, but not yet defined.  If we call getBlockValue and it
> returns undefined then the value is now on its second try with the given BB
> and we should stop there before adding it again to the map.
>
> So I think i’m using undefined being returned from the cache in the same way
> you are using a set of the seen BB/Value pairs.  I’m happy with either
> approach in principle to be honest, although yours is cleaner.
>
>
> 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.
>
> I like the patch.  Given that ODCacheUpdater is only ever used in the places
> you’ve updated, i think we can just hide the majority of what you’ve done
> its its implementation.  So lines such as these will actually be unchanged,
> and the code inside markResult could hopefully be changed instead.
>
>
> Yes, I'll try to do that. I think the main thing is I need to
> benchmark my patch for performance and add tests. I might not get
> around to that until Monday though.
>
> Sounds great. Thanks!

I've sent out http://reviews.llvm.org/D6397 for review. Please take a look!

 - Hans