[LLVMdev] [PATCH] PR2218

[Chris Lattner]

On Sep 2, 2009, at 1:07 AM, Jakub Staszak wrote:

> Hello,
>
> I fixed my patch as you asked. Sorry for the delay, I'd been working  
> on my SSU patch (http://lists.cs.uiuc.edu/pipermail/llvmdev/2009-August/025347.html 
> )
>
> I hope that everything is fine now.

Hey Jakub,

Thanks for working on this again, one more round :)

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.



+  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.




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

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.




+  if (CallInst *C = dyn_cast<CallInst>(dep.getInst())) {
+    CallSite CS = CallSite::get(C);
+    CS.setArgument(argI, Repl);

pointerIsParameter strips bitcasts.  Because of that, the type of the  
call argument may not be the same as the type of "repl", which could  
cause an assertion failure.  We need to either make pointerIsParameter  
not strip bitcasts, or have this code insert a bitcast when the types  
don't line up.

-Chris