[Patch] Enable value forwarding for loads from calloc without intervening store

[Duncan P. N. Exon Smith]

> On 2014-Jun-10, at 11:07, Philip Reames <listmail at philipreames.com> wrote:
> 
> ping.
> 
> Duncan - it looks like another notification may have gotten lost. Did you see my updated changes in response to your comments?
> 
> Philip

Thanks for the ping.  I never saw the updated patch.  Can you attach it?

> 
> 
> On 05/28/2014 10:48 AM, Philip Reames wrote:
>> 
>> On 05/28/2014 10:21 AM, Duncan P. N. Exon Smith wrote:
>>> On 2014 May 28, at 09:34, Philip Reames <listmail at philipreames.com> wrote:
>>> 
>>>> Enable value forwarding for loads from calloc without intervening store
>>>> http://reviews.llvm.org/D3931
>>>> 
>>>> This change extends GVN to handle the following case:
>>>> %1 = tail call noalias i8* @calloc(i64 1, i64 4) #2
>>>> %2 = bitcast i8* %1 to i32*
>>>> ; This load is trivially constant zero
>>>> %3 = load i32* %2, align 4
>>>> 
>>>> This is analogous to the handling for malloc in the same places. Malloc returns undef, calloc returns a zero value. Note that it is correct to return zero even for out of bounds geps since the result of such a gep would be undefined.
>>>> 
>>>> I'm not entirely clear why this code lives in GVN rather than elsewhere; I have chosen to simply extend the existing design.
>>> I agree that this is a strange spot, so you might want to wait to hear
>>> from someone else before further entrenching it.
>> I would prefer to do the rearrangement as a separate change.  I'm happy to contribute towards that; I just don't want to stall a useful enhancement on a broader design question.  If you seriously disagree, let me know.
>> 
>> Once I've uploaded the changed diff, would you mind committing it?
>>> 
>>> Otherwise, this LGTM with a few whitespace cleanups.
>> Will fix all.  Thanks.
>>> 
>>>> diff --git a/lib/Transforms/Scalar/GVN.cpp b/lib/Transforms/Scalar/GVN.cpp
>>>> index 6d07ddd..6250f2b 100644
>>>> --- a/lib/Transforms/Scalar/GVN.cpp
>>>> +++ b/lib/Transforms/Scalar/GVN.cpp
>>>> @@ -1463,6 +1463,12 @@ void GVN::AnalyzeLoadAvailability(LoadInst *LI, LoadDepVect &Deps,
>>>> UndefValue::get(LI->getType())));
>>>>       continue;
>>>>     }
>>>> +    // calloc zero initializes memory
>>> Comment should probably be preceded by an empty line, and should have
>>> a period.  Something like this would match the nearby comments:
>>> 
>>>    // Loading zero-initialized allocation -> 0.
>>> 
>>>> +    if (isCallocLikeFn(DepInst, TLI) ) {
>>> This space is strange ~~~~~~~~~~~~~~~~~^
>> Results from switching coding styles mid path.  I generally prefer if( cond ) { .. } which is not LLVM style.  Fixed.
>>> 
>>>> + ValuesPerBlock.push_back(AvailableValueInBlock::get(DepBB,
>>>> + Constant::getNullValue(LI->getType())));
>>>> +      continue;
>>>> +    }
>>>> 
>>>>     if (StoreInst *S = dyn_cast<StoreInst>(DepInst)) {
>>>>       // Reject loads and stores that are to the same address but are of
>>>> @@ -1988,6 +1994,14 @@ bool GVN::processLoad(LoadInst *L) {
>>>>     }
>>>>   }
>>>> 
>>>> +  // Loading from calloc returns zero
>>> This comment could be improved, too.  Add a period, and match the
>>> prose style of the comments nearby.
>>> 
>>>> +  if (isCallocLikeFn(DepInst, TLI)) {
>>>> + L->replaceAllUsesWith(Constant::getNullValue(L->getType()));
>>>> +    markInstructionForDeletion(L);
>>>> +    ++NumGVNLoad;
>>>> +    return true;
>>>> +  }
>>>> +
>>>>   return false;
>>>> }
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>