[LLVMdev] [Patches] Some LazyValueInfo and related patches

[Olivier Goffart]

On Sunday 26 January 2014 23:02:33 Nick Lewycky wrote:
> Olivier Goffart wrote:
> > Hi.
> > 
> > Attached you will find a set of patches which I did while I was trying to
> > solve two problems.
> 
> Patches should go to the llvm-commits list, not llvmdev. (Think of it as
> "llvm-patches" if that helps.)


Thank for your comments. Please have a look at my answer bellow.

> 
> > I did not manage to solve fully what i wanted to improve, but I think it
> > is
> > still a step in the right direction.
> > 
> > The patches are hopefully self-explanatory.
> > The biggest change here is that LazyValueInfo do not maintain a separate
> > stack of work to do,
> > but do the work directly recursively.
> 
> That sounds scary. We often get crash reports where we have recursion,
> and replace it with a worklist stack. The difference is purely
> mechanical. Is there a reason you needed to do this?
> 
> > The test included in the patch 4 also test the patch 2.
> 
> For patch 1 (SCCP):
> 
> -  // load null -> null
> -  if (isa<ConstantPointerNull>(Ptr) && I.getPointerAddressSpace() == 0)
> -    return markConstant(IV, &I, Constant::getNullValue(I.getType()));
> 
> This patch doesn't include a test so it isn't clear why you removed
> this. The transform you remove is correct -- load of null in addrspace 0
> is undefined behaviour. We could model the load as returning undef, but
> undef's nature makes it hard to reason about in SCCP (if one transform
> relies on a particular bit being 0 and another transform relies on a
> particular bit being 1, we won't notice and will miscompile), so picking
> null as a result seems reasonable?

I forgot what the problem was. I was trying to understand why code paths that 
accessed null pointer was not removed,  and found out that this pass was 
replacing the undefined behaviour by 0, before some other pass was treating it 
as an undefined behaviour and removed the whole chunk of code.  But i don't 
have anymore the test case for it.
Actually, that patch is not that important.

> For patch 2:
[...]
> inferFromCondition is directly recursive, please make it use a worklist.
> It doesn't need to exit and go through solveBlock, it can just keep its
> worklist local to its own function.

Aren't worklists slower and more complicated?
Is the problem the risk of a stack overflow?  In that case, is it not better 
then to limit the number of recursion to some limit (but what then?)


> For patch 4:
> 
> Sorry, but this isn't acceptable as is, you'll have to find a different
> approach. Please find a way to make it work within the framework of LVI.
> 
> Why doesn't the existing recursion work? As far as I can tell, you ought
> to be getting the same result without this patch, and I think that
> suggests there's a bug elsewhere in LVI that we need to find instead.
> Returning false ought to redo the relevant computations.
> 
> It's hard for me to see it fail because the testcase depends on patches
> 1 through 3, otherwise I would fire up a debugger and try to sort it out

Let's take the example of the test. We want to compute the range of %off1 in 
block "do".

In LazyValueInfoCache::sloveBlockValue,  one of the first thing that is done 
is:   BBLV.markOverDefined(); 
Since BBLV is a reference to the cache, this actually write into the cache.
Then we realize that we need to compute the range of %a. so we put %a into the 
worklist, return false and "forget" all the work we already did for %off1.
Then the range of %a will be re-computed.
Then we will retry to compute %off1,  but since it is already in the cache as 
overdefined, we stop there and return it is overdefined.

However, if we were not marking this as overdefined, it breaks once there is a 
loop and the range of a value depends on itself.

Are recursive function really forbidden in LLVM?
Should a depth limit be acceptable.


Regards, 
-- 
Olivier