[llvm-commits] [llvm] r90045 - in /llvm/trunk: lib/Analysis/MemoryDependenceAnalysis.cpp test/Transforms/DeadStoreElimination/lifetime.ll
Chris Lattner
clattner at apple.com
Tue Dec 1 13:35:54 PST 2009
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
More information about the llvm-commits
mailing list