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