[PATCH] D23214: [MDA] Treat invariant.start as non-dependence

[Daniel Berlin via llvm-commits]

On Fri, Aug 5, 2016 at 1:11 PM, Sanjoy Das <sanjoy at playingwithpointers.com>
wrote:

> Hi Daniel,
>
> Daniel Berlin wrote:
> > Yes, i'm saying the  mechanism currently used to keep them from moving
> > is to say they read/write memory.
> >
> >     That
> >     does not seem safe -- how does LLVM know that they won't segfault?
> >
> >
> >
> > I'm not sure i understand.
>
> I meant given a call that is readonly, LLVM cannot generally hoist it
> out of control flow unless it has special knowledge that the call does
> not read invalid memory.



Okay, so you are saying you don't believe it will cause the same problem
because you don't believe anything will legally try to hoist these.

I'm willing to find out!

Otherwise it will introduce undefined
> behavior:


I'm not opposed to trying to mark these as readonly, but realize you are
also assuming you know what the real invariant range is and what causes it
:)

Given the following:

store(something that LLVM does not think aliases foo)
bar = invariant.start(foo, 5)
load foo
invariant.end(bar)

Transforming it into:

bar = invariant.start(foo, 5)
store(something that LLVM does not think aliases foo)
load foo
invariant.end(bar)

Is equally as illegal by the langref, since you are now saying "during the
operation of this store, the memory for foo is invariant", which is not
what it said originally (though i can't think of cases this will break that
don't involve threading).

I also don't honestly know if getModRefInfo is going to say this is legal
or not.

That said, in theory:

%a = add 5, 3
bar = invariant.start(foo, 5)
load foo
invariant.end(bar)

to
bar = invariant.start(foo, 5)
%a = add 5, 3
load foo
invariant.end(bar)

is probably also illegal, and has the same issues :)

Now, i have no desire to be that pedantic (i'd rather just update the
langref).






>   if (always_false)
>     readonly_call_that_derefs_null()
>
> ==>
>
>   readonly_call_that_derefs_null()
>   if (always_false)
>     ...
>
> > These are invariant start and end markers, they generate no real code.
> > If you move them, the problem is that the definition of invariant.start
> > says:
> > "This intrinsic indicates that until an|llvm.invariant.end|that uses the
> > return value, the referenced memory location is constant and unchanging.
> > "
> >
> > So if you hoist/sink start or end , you change the bounds of where the
> > memory is unchanging.
>
> Yes. I can see why cannot move _stores_ across them, since
>
>
>   *ptr = 20
>   invariant_start(ptr)
>
> ==>
>
>   invariant_start(ptr)
>   *ptr = 20
>
>
> is bad -- we've moved a store to a region in which ptr needs to be
> unchanging.
>
>
> With the memdep changes we will allow reordering loads across the
> invarinat_start call:
>
>   int val = *ptr;
>   invariant_start(ptr)
>
> <==>
>
>   int val = *ptr;
>   invariant_start(ptr)
>
>
(you meant for the first one to have the load after invariant start, i
presume)


> but that seems fine to me; that is I could not find a reason why
> reordering loads across invariant_start will be a problem.


Me either :)


> But I
> don't have a deep reason why reordering loads is correct, so I won't
> be surprised if there are cases that reordering is a problem.


I would be surprised if someone does not find a case where it is :)


>
> > You could fix this, of course, by having the invariant.start and
> > invariant.end calls return pointers, and all things that are invariant
> > use those pointers (or pointers derived from those pointers).  IE
> > express the dependence in SSA directly.
>
> I remember a thread by Andy Trick a while back which intended to add
> this to LLVM.  But that didn't result in concrete patches.
>
> > Then you can move the start/end calls wherever they are valid (IE
> > wherever they are dominated by their operands) and it will not change
> > the semantic effect.
> >
> > But that's not the current design.
> >
> >
> >     >  We last tried this by saying they did not touch memory (which is
> >     >  correct), and peter had to revert the set of changes that gave
> >     them "The
> >     >  correct answer" because it broke on real code.
> >
> >     Do you happen to remember any relevant revision numbers / zilla bugs
> >     for this?
> >
> > The revert was: "[llvm] r270823 - MemorySSA: Revert r269678 and r268068;
> > replace with special casing in MemorySSA.
> > "
> >
> > This is when we tried this with assume
>
> Oh, I remember that one.  But IIUC that was not a problem with
> speculation but with DCE'ing away assumes.  ReadNone can't be
> speculated in LLVM either since they can contain a divide-by-zero.
>
> I don't think that the same problem will occur here since unlike the
> assume patch, we're not telling LLVM that calls to invariant_start are
> readonly, but that they don't cause a dependency on any specific load
> instruction in the LLVM IR.  This is sort of like saying that the
> invariant_start call does write to memory, but just not to memory that
> can be observed via LLVM IR.
>

I'm willing to give it a shot if you want, i'm just in the "we'll probably
break something unexpected" camp :)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160805/2ff35d78/attachment.html>