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

[Hal Finkel]

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