[llvm-commits] [llvm] r90045 - in /llvm/trunk: lib/Analysis/MemoryDependenceAnalysis.cpp test/Transforms/DeadStoreElimination/lifetime.ll

[Chris Lattner]

On Nov 28, 2009, at 1:27 PM, Nick Lewycky wrote:

> Author: nicholas
> Date: Sat Nov 28 15:27:49 2009
> New Revision: 90045
>
> URL: http://llvm.org/viewvc/llvm-project?rev=90045&view=rev
> Log:
> Teach memdep to look for memory use intrinsics during dependency  
> queries. Fixes
> PR5574.

Hi Nick,

I'm just now catching up on all of the lifetime/invariant stuff you've  
added to memdep, here are some comments:

memdep:188:

     if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(Inst)) {
       // If we pass an invariant-end marker, then we've just entered an
       // invariant region and can start ignoring dependencies.
       if (II->getIntrinsicID() == Intrinsic::invariant_end) {
         uint64_t InvariantSize = ~0ULL;
         if (ConstantInt *CI = dyn_cast<ConstantInt>(II->getOperand(2)))
           InvariantSize = CI->getZExtValue();

Operand 2 is guaranteed to be a ConstantInt, this should use cast<>,  
however...

         AliasAnalysis::AliasResult R =
           AA->alias(II->getOperand(3), InvariantSize, MemPtr, MemSize);
         if (R == AliasAnalysis::MustAlias) {
           InvariantTag = II->getOperand(1);
           continue;
         }

mustalias queries don't care about the size of the operands, so  
getting it is pointless.  Either the pointers themselves mustalias or  
not.  As such, this won't catch the case where you have a lifetime  
marker saying that a 20 byte object is live/dead but where you're  
accessing 4 bytes into the 20 byte object.  I don't know if you care  
about this case, if so, please fix it somehow, otherwise remove the  
dead code and add a FIXME describing the issue.


In the case when you do have a mustalias, why do you continue the scan  
at all?  Can't you just look at the instruction that defines the  
Invariant tag and jump up to it?  That would allow eliminating  
InvariantTag entirely.  I really don't like how InvariantTag is  
scattered throughout getPointerDependencyFrom.


       } else if (II->getIntrinsicID() == Intrinsic::lifetime_start ||
                  II->getIntrinsicID() == Intrinsic::lifetime_end) {

This has the same problem as above, cast<> and mustalias.

         if (R == AliasAnalysis::MustAlias)
           return MemDepResult::getDef(II);

Returning a def of the lifetime start/end is perfectly reasonable  
here, but please please document this case (the semantics of this  
case) in the comment above the MemDepResult::Def enum in MemDep.h.


     // Debug intrinsics don't cause dependences.
     if (isa<DbgInfoIntrinsic>(Inst)) continue;

This can be hoisted into the 'if dyn_cast<intrinsicinst>' block.


memdep:378:
     switch (IntrinsicID) {
       case Intrinsic::lifetime_start:
       case Intrinsic::lifetime_end:

Please indent the case to the level of the switch, and add a break at  
the end of the default case.

What does this code *do* anyway?  Does someone actually call  
getDependency() on a lifetime/invariant intrinsic?

     bool isLoad = !QueryInst->mayWriteToMemory();
     if (IntrinsicInst *II = dyn_cast<MemoryUseIntrinsic>(QueryInst)) {
       isLoad |= II->getIntrinsicID() == Intrinsic::lifetime_end;
     }

This dance is gross.

-Chris