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

[Kenneth Uildriks]

On Mon, Nov 29, 2010 at 1:20 PM, Dan Gohman <gohman at apple.com> wrote:
>
> 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.

My understanding from previous discussions was that llvm.invariant is
a function local thing, so that construct would not be recognized.
You can always outline code inside of the llvm.invariant markers
without outlining the invariant markers themselves.

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

I believe a given query uses alias analysis to determine whether an
invariant begin/end marker applies to the pointer being queried.

>
> Here are a few specific comments on the patch:
>
>> +  struct InvariantInfo {
>> +    unsigned Size;
>
> AliasAnalysis uses uint64_t for sizes now.

OK, I'll change it.

>
>> +      else if (F->getName() == "llvm.invariant.start"
>
>> +      else if (F->getName() == "llvm.invariant.end"
>
> These should use F->getIntrinsicID().

Thanks for the heads up.  I'll change that too.

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

Beause we don't know yet whether the specific instruction inside the
block is inside of an invariant region or not.  The load and the store
might both be in the invariant region, and since we start scanning
backward from the load, we don't know whether that's the case until we
see an invariant marker.

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

I had meant to include two testcases... I'll try again.