[LLVMdev] [PATCH] PR2218

[Jakub Staszak]

On Sep 2, 2009, at 3:15 PM, Chris Lattner wrote:

> Please merge the three testcases into one file.  We added a new  
> FileCheck tool which allows you to check for the exact sequence of  
> instructions expected, which also allows the tests to be merged into  
> one file.
>
> +/// MemCpyOpt::pointerIsParameter - returns true iff pointer is a  
> parameter of
> +/// C call instruction.
> +bool MemCpyOpt::pointerIsParameter(Value *pointer, CallInst *C,  
> unsigned &argI)
> +{
> +    CallSite CS = CallSite::get(C);
> +    for (argI = 0; argI < CS.arg_size(); ++argI)
>
> Please make this a static function, it doesn't need "this".  Also,  
> please do something like this in the for loop:
>
> +    for (argI = 0, CS.arg_size(); argI != argE; ++argI)
>
>
> pointerIsParameter returning a bool and the argument # byref is  
> somewhat awkward.  I think it would be better if it returned an int,  
> which was -1 if not found.

The problem is that CS.arg_size() is "unsigned int". Of course, I can  
cast int -> unsigned int but it wouldn't look good I think.


>
> +  MemoryDependenceAnalysis& MD =  
> getAnalysis<MemoryDependenceAnalysis>();
>
> You can sink this link to right before the call to MD.getDependency.
>
>
>
> +  // Require pointer to have local dependency.
> +  if (!dep.getInst() || dep.isNonLocal())
> +    return false;
>
> Random thought: after this patch goes in, it would be interesting to  
> see how many queries get rejected because they are non-local:  
> handling non-local would be pretty easy.  Please add a TODO that  
> mentions this.
>
> In any case, the check for dep.isNonLocal() can be removed.   
> Nonlocal queries have a null instruction.

OK, i'll work on it.

>
> +  } else if (StoreInst *S = dyn_cast<StoreInst>(dep.getInst())) {
> ...
> +    uint64_t ptrSize = AA.getTypeStoreSize(pointer->getType());
> ...
> +    uint64_t memSize = AA.getTypeStoreSize(memPtr->getType());
> ...
> +    if (AA.alias(pointer, ptrSize, memPtr, memSize) !=  
> AliasAnalysis::MustAlias)
>
> This is passing in the size of the pointer, not the pointee.   
> However, since you only care about must alias, you don't care about  
> the size of the object, you can just pass in 1 for both sizes.

Ahh, last time you said I had to check sizes. You said:
"Knowing that the loaded and stored pointer must alias is important,  
but you also need to check that the loaded and stored sizes equal each  
other." :-)



> Please put a comment above the call to AA.alias explaining why you  
> require mustalias here.
>
>
>
> +  // Look for a replacement for our pointer.  If more than one  
> found, exit.
> +  if (!L->hasOneUse())
> +    return false;
> +  StoreInst *StoreOfLoad = dyn_cast<StoreInst>(L->use_back());
> +  if (!StoreOfLoad)
> +    return false;
>
> Please do these check before the MemDep query, they are very cheap  
> and will make the optimization run faster (by avoiding queries).
>
>
>
> I'm pretty sure that there is still a legality check missing here.   
> Your code will transform this:
>
>   %temporary = alloca i8
>   call void @initialize(i8* noalias sret %temporary)
>   %tmp = load i8* %temporary
>   store i8 42, i8* %result
>   store i8 %tmp, i8* %result
>
> to:
>
>   %temporary = alloca i8
>   call void @initialize(i8* noalias sret %result)
>   store i8 42, i8* %result
>
> which is a miscompilation.  To fix this, I'd do a memdep query of  
> "result" from StoreOfLoad and from L.  If they both have the same  
> dependence, then the transform is ok.

See PR2218-dont.ll test.


-Jakub