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

[Philip Reames]

ping.

Duncan - it looks like another notification may have gotten lost. Did 
you see my updated changes in response to your comments?

Philip


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