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

[Sanjoy Das via llvm-commits]

Hi Daniel,

Daniel Berlin wrote:
 > 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.

Yes (unless we special case invariant_* intrinsics some other way).

 > 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).

(If!) I understand what you're saying, I was not able to come with a
concrete multi-threaded situation where re-ordering non-aliasing
stores will be a problem, as long as we respect
acquire/release/seq_cst re-ordering restrictions a readonly call would
have to obey.  This is the closest example I could come up with:

// ptr0 and ptr1 are noalias, *ptr0 == 0, *thread_b_start = false
// initially, all accesses are relaxed.

ThreadA:
   *ptr0 = 50
   fence seq_cst
   *thread_b_start = true
   *ptr1 = 100
   invariant_start(ptr1, sizeof(*ptr0))
   **global = 90
   int val = *ptr1

ThreadB:
   while (!*thread_b_start) { fence seq_cst; }
   fence seq_cst
   if (*ptr0 != 50)
     global = ptr1

The idea being we can certify *ptr1 to be invariant only once we've
published the *ptr0 = 50 store.  If that gets re-ordered with the
invariant_start call then we're in trouble because the location would,
in fact, be changed by ThreadB while ThreadA was supposed to observe
an invariant *ptr1.  However, I'd expect the "fence seq_cst" to
prevent that re-ordering.

 > 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 :)

I don't understand why that ^ is a problem.

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

I meant given what we're teaching MDA, the two are equivalent (hence
the "bidirectional arrow" :)).

 > I'm willing to give it a shot if you want, i'm just in the "we'll
 > probably break something unexpected" camp :)

I now think it is better to add one more special case to AliasAnalysis
around invariant_start s, like we do for assumes.  It's a hack, but at
least a localized hack.

-- Sanjoy