[LLVMdev] [PATCH] PR2218

[Jakub Staszak]

Hello,

Sorry for my stupid mistakes. I hope that everything is fine now. This  
patch fixes PR2218. There are no loads in example, however  
"instcombine" changes memcpy() into store/load.

Regards,
Jakub Staszak
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr2218-2.patch
Type: application/octet-stream
Size: 6525 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090725/b7dd0f10/attachment.obj>
-------------- next part --------------


On Jul 22, 2009, at 10:51 PM, Chris Lattner wrote:

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