[llvm-commits] [llvm] r85383 - in /llvm/trunk: lib/Analysis/MemoryDependenceAnalysis.cpp lib/Transforms/Scalar/DeadStoreElimination.cpp lib/Transforms/Scalar/GVN.cpp test/Transforms/DeadStoreElimination/lifetime-simple.ll test/Transforms/GVN/lifetime-simple.ll

[Chris Lattner]

On Oct 28, 2009, at 12:05 AM, Owen Anderson wrote:

> Author: resistor
> Date: Wed Oct 28 02:05:35 2009
> New Revision: 85383
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=85383&view=rev
> Log:
> Treat lifetime begin/end markers as allocations/frees respectively for the
> purposes for GVN/DSE.

Hi Owen, some questions:

> +++ llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp Wed Oct 28 02:05:35 2009
> @@ -200,6 +199,19 @@
>           invariantTag = II->getOperand(1);
>           continue;
>         }
> +      
> +      // If we reach a lifetime begin or end marker, then the query ends here
> +      // because the value is undefined.
> +      } else if (II->getIntrinsicID() == Intrinsic::lifetime_start ||
> +                   II->getIntrinsicID() == Intrinsic::lifetime_end) {
> +        uint64_t invariantSize = ~0ULL;
> +        if (ConstantInt* CI = dyn_cast<ConstantInt>(II->getOperand(1)))
> +          invariantSize = CI->getZExtValue();
> +
> +        AliasAnalysis::AliasResult R =
> +          AA->alias(II->getOperand(2), invariantSize, MemPtr, MemSize);
> +        if (R == AliasAnalysis::MustAlias)
> +          return MemDepResult::getDef(II);
>       }
>     }

I just pointed out several issues with this code (I thought Nick wrote it) here:
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20091130/092042.html

Beyond what I wrote there, I Don't see why memdep is caring about handling a load that depends on a lifetime end.  This should *never* happen, so why do we even have the code?

> +++ llvm/trunk/lib/Transforms/Scalar/GVN.cpp Wed Oct 28 02:05:35 2009
> @@ -1248,6 +1248,15 @@
>                                              UndefValue::get(LI->getType())));
>       continue;
>     }
> +    
> +    // Loading immediately after lifetime begin or end -> undef.
> +    if (IntrinsicInst* II = dyn_cast<IntrinsicInst>(DepInst)) {
> +      if (II->getIntrinsicID() == Intrinsic::lifetime_start ||
> +          II->getIntrinsicID() == Intrinsic::lifetime_end) {
> +        ValuesPerBlock.push_back(AvailableValueInBlock::get(DepBB,
> +                                             UndefValue::get(LI->getType())));
> +      }

Likewise, this should not care about lifetime_end.  It is also missing a continue, which I fixed on mainline.

> +  
> +  // If this load occurs either right after a lifetime begin or a lifetime end,
> +  // then the loaded value is undefined.

Again, why lifetime_end?

> +  if (IntrinsicInst* II = dyn_cast<IntrinsicInst>(DepInst)) {

Please fix the *.

-Chris