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

[Nick Lewycky]

Chris Lattner wrote:
>
> 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.

Thanks. I don't really have an intuitive understanding of what Def 
really means in MemDep.

> // 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?

Sure. In test/Analysis/BasicAA/modref.ll, DSE runs through every 
instruction in @test3 and calls getDependency(Inst) on each one. DSE 
bails (on the instruction) if it doesn't get a Def back.

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

I hate it too. Shall I factor it into a function? It feels like that's 
not an improvement, we're just moving badness around.

Nick