[LLVMdev] [PATCH] PR2218

[Chris Lattner]

On Jul 25, 2009, at 4:48 PM, Jakub Staszak wrote:

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

Hi Jakub,

Sorry for the delay, I'm way behind on code review.  Generally if you  
respond quickly, I'll remember enough about the previous email that I  
can respond quickly too.  If it gets out of my brain then it takes me  
a while to get back to it.  I'll try to be better in the future :(

This patch is looking like a great improvement.  Some comments on  
formatting:

Please pull this out to a helper function:
+    CallSite CS = CallSite::get(C);
+
+    // Pointer must be a parameter (case 1)
+    for (argI = 0; argI < CS.arg_size(); ++argI)
+      if (CS.getArgument(argI)->stripPointerCasts() == pointer)
+	break;
+
+    if (argI == CS.arg_size())
+      return false;

per http://llvm.org/docs/CodingStandards.html#hl_predicateloops


+    // Store cannot be volatile (case 2) and must-alias with our  
pointer.
+    if (S->isVolatile()) {
+      return false;
+    } else {

no need for the 'else after return': http://llvm.org/docs/CodingStandards.html#hl_else_after_return

+      AliasAnalysis& AA = getAnalysis<AliasAnalysis>();
+      if (AA.alias(S->getPointerOperand(), 1, pointer, 1) !=
+	  AliasAnalysis::MustAlias)
+	return false;

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.

+  // Look for a replacement for our pointer. If more than one found,  
exit.
+  for (User::use_iterator I = L->use_begin(), E = L->use_end(); I !=  
E; ++I) {
+    Instruction *User = cast<Instruction>(*I);
+
+    if (StoreInst *SI = dyn_cast<StoreInst>(User)) {
+      if (SI->isVolatile())
+	return false;
+
+      if (!Repl)
+	Repl = SI->getPointerOperand();
+      else if (Repl != SI->getPointerOperand())
+	return false;
+
+    } else
+      return false;
+  }

Please do something like this:

if (!L->hasOneUse()) return false;
StoreInst *StoreOfLoad = dyn_cast<StoreInst>(L->use_back());
if (StoreOfLoad == 0 || ...)
...


Actually, I see now that the code actually allows multiple stores as  
long as they are to the same pointer.  That also seems reasonable to  
me, but please update the comment above the loop to make it really  
clear what it is doing.  It is probably also reasonable to factor this  
out to a static helper function.

I'm still not sure that this transformation is safe (it seems like we  
need another dependence query).  In particular, given something like:

     %temporary = alloca i8
     call void @initialize(i8* noalias sret %temporary)
     %tmp = load i8* %temporary
     store 42 -> %result
     store i8 %tmp, i8* %result


It is not safe to transform this into:

     call void @initialize(i8* noalias sret %result)
     store 42 -> %result


I think this can be fixed by doing a dependence query from the store,  
though I'm not sure what we'd expect.  Another option is to see if the  
result is only used by the store.  In this case, the loop above that  
allows multiple stores to the same pointer seems unneeded because we'd  
only be able to support one store.

Thanks for working on this!

-Chris