[llvm-commits] [llvm] r47265 - in /llvm/trunk: lib/Transforms/Scalar/GVN.cpp test/Transforms/GVN/sret.ll

Duncan Sands baldrick at free.fr
Mon Feb 18 09:54:46 PST 2008


Hi Owen, nice transform!

> +  // Check that we're copying to an argument...
> +  Value* cpyDest = cpy->getDest();
> +  if (!isa<Argument>(cpyDest))
> +    return false;
> +  
> +  // And that the argument is the return slot
> +  Argument* sretArg = cast<Argument>(cpyDest);
> +  if (!sretArg->hasStructRetAttr())
> +    return false;

why is it relevant that the memcpy is to an sret argument?
The transform would be equally valid if it was to a temporary
I think.  The essential condition seems to be: the memcpy
destination should not be accessable (for read or write) by
the function called.  Knowing that the destination is a
noalias parameter is simply helpful in determining that (I
don't see what sret has to do with it...).

On the other hand, it matters that the memcpy source is
passed to an sret parameter, because this means that the
called function does not read any existing values in it
(i.e. it is undefined what the callee gets if it tries to
read existing values via the sret parameter).  OK, I know
this is not explicitly part of the sret definition, but it
could reasonably be.  The patch doesn't seem to check
whether the memcpy source is being passed as an sret parameter.

Finally, it matters that the size of the memcpy is the
whole size of the memcpy source.  Imagine that you pass
a 10 byte memtmp to the call, then memcpy 5 bytes of it
to a 5 byte object.  It is clearly wrong to pass the
5 byte object directly to the call.

Ciao,

Duncan.

PS: The other conditions like no other uses also seem way
too strict.



More information about the llvm-commits mailing list