[llvm-commits] [PATCH] Fix llvm.invariant support

[Dan Gohman]

On Nov 22, 2010, at 7:14 PM, Kenneth Uildriks wrote:

> Ping.
> 
> On Sat, Nov 13, 2010 at 7:00 PM, Kenneth Uildriks <kennethuil at gmail.com> wrote:
>> A less drastic attempt to fix issues with support for
>> llvm.invariant.start and llvm.invariant.end.  It involves adding
>> invariant tracking by block to MemoryDependenceAnalysis.  When a load
>> is analyzed, the invariant information for that block and its
>> predecessors is lazily computed and consulted.  If the load's target
>> is not invariant anywhere in the block, there is no need to look for
>> invariant markers during the scan.  If it is invariant throughout the
>> block, no def or clobber can be in that block, so we can skip
>> backwards to the block(s) that began the invariance.  If it is
>> invariant through part of the block, we need to continue scanning
>> backwards past clobbers until we find the def or an invariant marker -
>> we can't assume that the scan starts after the invariant end, but
>> instead assume that we *might* have started scanning in the middle of
>> an invariant region until we can prove otherwise.

How do @llvm.invariant.start and @llvm.invariant.end work?

There are at least two interesting situations.

First, is this valid?

define void @callee(i8* %p) {
 call void @llvm.invariant.end(i8* %p)
 ret void
}

If so, then any call can call @llvm.invariant.end on any pointer it has
access to, which substantially weakens what MemDep can do. If not, then
it seems to violate high-level IR design principles -- it should always
be valid to out-line code regions.

Next, what about this?

  %a = alloca i32
  %b = bitcast i32* %a to i8*
  %c = call {}* @llvm.invariant.begin(i8* %b)
  %d = bitcast {}* %c to i8*
  call void @llvm.invariant.end(i8* %d)

If I read your patch correctly, it won't notice that the invariant
region for %b is ended here, because it's ended with a different
pointer. The results for subsequent getPointerDependencyFrom queries
will depend on which pointer happens to land first in the std::map.
Or did I miss something?

Here are a few specific comments on the patch:

> +  struct InvariantInfo {
> +    unsigned Size;

AliasAnalysis uses uint64_t for sizes now.

> +      else if (F->getName() == "llvm.invariant.start"

> +      else if (F->getName() == "llvm.invariant.end"

These should use F->getIntrinsicID().

> @@ -321,8 +367,11 @@
> 
>        if (R == AliasAnalysis::NoAlias)
>          continue;
> -      if (R == AliasAnalysis::MayAlias)
> -        return MemDepResult::getClobber(Inst);
> +      if (R == AliasAnalysis::MayAlias) {
> +        if (!InvariantInside)
> +          return MemDepResult::getClobber(Inst);
> +        MayBeClobbered = true;
> +      }
>        return MemDepResult::getDef(Inst);
>      }

It's not obvious what's happening here. If the pointer is
invariant inside the block, why is it marked MayBeClobbered?

And, a testcase demonstrating the basic functionality would
be helpful.

Dan