[LLVMdev] [Patches] Some LazyValueInfo and related patches

[Nick Lewycky]

Olivier Goffart wrote:
> 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.

Okay. I'll explicitly NAK that patch as something which has been 
reviewed and should not be submitted.

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

Not really, it's exactly the same as what we were going to put on the 
stack, but on the heap instead. We're just trading memory at one pointer 
for one at another.

This is usually as simple as wrapping your function in:
   SmallVector<T, 4> WorkList;
   WorkList.push_back(V);
   do {
     T t = WorkList.pop_back_val();
     // ... work here ...
   } while (!WorkList.empty());

and pushing onto the worklist in every place you'd recurse. It's a 
mechanical transformation, there shouldn't be any significant impact on 
performance or complexity.

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

Careful. When you start doing that you end up with seemingly irrelevant 
changes to a function triggering cascading effects on whether/how the 
function gets optimized. Yes there are places we do this in LLVM, but it 
is never my first choice.

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

(Sorry, I need to dedicate a block of time to go through this, but I've 
had the first half of this email sitting in drafts for weeks now, so 
I've decided to send that and will get to this part later.)

Nick

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