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

[Philip Reames]

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;
>> }