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

[Duncan P. N. Exon Smith]

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.

Otherwise, this LGTM with a few whitespace cleanups.

> 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 ~~~~~~~~~~~~~~~~~^

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