[LLVMdev] [PATCH] PR2218

[Chris Lattner]

On Jul 22, 2009, at 1:37 PM, Jakub Staszak wrote:

> Hello,
>
> This patch fixes PR2218.

Very nice.  Are you sure this fixes PR2218?  The example there doesn't  
have any loads in it.

>  However, I'm not pretty sure that this optimization should be in  
> MemCpyOpt. I think that GVN is good place as well.

Yes, you're right.  My long term goal is to merge the relevant pieces  
of memcpyopt into GVN/DSE at some point.  To do that, some more major  
surgery needs to be done to memdep to make it work both backward (to  
support GVN) and forward (to support DSE).  In the meantime, enhancing  
memcpyopt is fine, so long as there are good testcases (and your patch  
has them!).

Nit picky stuff about the patch:

+    for(unsigned argI = 0; argI < CS.arg_size(); ++argI) {
+      if(CS.getArgument(argI)->stripPointerCasts() == pointer)

Please put spaces after if/for.  You get this right in some places,  
but wrong in others.

+  Value* pointer = L->getPointerOperand();

Please use "Value *pointer" style throughout.  It looks like MemCpyOpt  
is already really inconsistent about this :-/ but please don't make it  
worse :).

+    /* Pointer must be a parameter (case 1) */

please use C++ style // comments.



Some other points:
1. I don't see a check that the load isn't volatile, please don't hack  
volatile loads.

2. The dependency check is relatively expensive, so please reorder it  
to be checked after the check for alloca:

+  Value* pointer = L->getPointerOperand();
+  MemDepResult dep = MD.getDependency(L);
+
+  // Require pointer to have local dependency.
+  if(!dep.getInst() || dep.isNonLocal())
+    return false;
+
+  // Require pointer to be an alloca.
+  if(!isa<AllocaInst>(pointer))
+    return false;


3. In this code, does the argument also need to be sret, or is any  
argument ok?

+  if(CallInst *C = dyn_cast<CallInst>(dep.getInst())) {
+    CallSite CS = CallSite::get(C);
+
+    /* Pointer must be a parameter (case 1) */
+    if(!CS.hasArgument(pointer))
+      return false;

4. Here, please do an alias check if the store's pointer operand is  
not the same as the load's (allowing the xform if they are must- 
alias).  This helps in some cases where there are GEP's that are the  
same but not actually CSE'd:

+    /* Store cannot be volatile (case 2) */
+    if(S->getPointerOperand() != pointer || S->isVolatile())
+      return false;


5. In the logic for your xform, I don't see any check to see if the  
alloca itself has uses other than the load/store.  In this example,  
won't it miscompile the code by eliminating the store into temporary?

    %temporary = alloca i8
    call void @initialize(i8* noalias sret %temporary)
    %tmp = load i8* %temporary
    store i8 %tmp, i8* %result
    %tmp2 = load i8* %temporary
       ...

6.  In the xform part of the code, you strip pointer casts, but you  
don't do that in the argument checking part, why?

+    for(unsigned argI = 0; argI < CS.arg_size(); ++argI) {
+      if(CS.getArgument(argI)->stripPointerCasts() == pointer)

Thanks for working on this area, I'm glad to see it getting attention!

-Chris