[PATCH] Scoped NoAlias Metadata

[hfinkel at anl.gov]

----- Original Message -----
> From: hfinkel at anl.gov
> To: hfinkel at anl.gov, dan433584 at gmail.com, atrick at apple.com, chandlerc at gmail.com
> Cc: "zinovy nis" <zinovy.nis at gmail.com>, mcrosier at codeaurora.org, nlewycky at google.com, "renato golin"
> <renato.golin at linaro.org>, nrotem at apple.com, llvm-commits at cs.uiuc.edu
> Sent: Wednesday, July 23, 2014 6:04:44 PM
> Subject: Re: [PATCH] Scoped NoAlias Metadata
> 
> I talked with Chandler on IRC last night, and he's satisfied that
> I've discussed this with Nick (which was his review requirements
> from long ago). As I've mentioned previously, the outcome of that
> discussion is that we need a sanitizer for aliasing. On this there
> is no disagreement, and I've started developing one (see
> http://reviews.llvm.org/D4446).
> 
> Rereading the code, however, there is bad news: The exact scheme
> proposed in this patch won't do what we want it to do, and will be
> hard for Clang to use effectively. First, I'll mention that the code
> here that transforms noalias function parameters into metadata
> during inlining is wrong, primarily because it omits a capturing
> analysis. This is fixable during inlining, but explains why Clang
> could not effectively use the scheme for block-level restrict
> pointers: Clang would need to do a capturing analysis to emit the
> metadata too (at least, if it were to do a good job of it).
> 
> To explain: If we have some function like:
> void foo(noalias a, noalias b) {
>   c = foo(a, b);
>   *c = *a + *b;
> }
> 
> In the scheme in this patch, we first define a 'scope' for this
> function for each noalias parameter (so here we have 2 scopes, one
> for a and one for b), so here we'd tag *a with !scope_a and *b with
> !scope_b (because a and b are their pointer's underlying objects).
> Then we'd look for the accesses: We tag *a with 'noalias !scope_b'
> (because its pointer does not derive from b), we'd tag *b with
> 'noalias !scope_a' similarly, and we'd tag *c with 'noalias
> !scope_a,!scope_b', which is wrong. Even though the pointer used by
> *c does not directly derive from a or b, if a or b are captured by
> foo, then c might be derived from them.
> 
> Within the scheme in this patch, there are two possible ways to fix
> this; perform a capture analysis either:
>   1. Be conservative and not place any metadata on *c.
>   2. Generate yet-another scope to represent 'derived from a or b',
>   and also tag everything that does not potentially derived from or
>   or b with this scope metadata also.
> Neither choice is ideal. In the first case, we discard useful
> information. In the second, we could have an
> exponentially-increasing about of metadata (which we'd need to
> cutoff at some point, leaving us the first choice).
> 
> In any case, there is a better scheme, which provides an ideal
> solution here and also is easily usable from Clang:
>  1. Keep the scope metadata, but only generate one scope per function
>  (or block if doing it from Clang).
>  2. Do not use noalias metadata, but add a @llvm.noalias(ptr*,
>  !metadata) intrinsic; this intrinsic will use the pointer value and
>  also associate its noalias assumption with the scope metadata to
>  which it applies.
>  3. As BasicAA ascends up the value chain, in BasicAA::aliasCheck, it
>  can look for users of the pointer that are @llvm.noalias, and if it
>  finds one, look at the scope metadata node from the Location
>  object, and if it finds a match, it treats that value as if it had
>  found a noalias argument.

As it turns out, unfortunately, this does not work (so I'm returning to the original design -- with the necessary added capture analysis during inlining). I'll explain why:

callee:
foo(noalias a) {
  a[-4] = *a;
}

caller:
x = w+4;
foo(x, m);

then we inline:
x = w+4
@llvm.noalias(x, !foo);
x[-4] <!foo> = *a <!foo>;

then we simplify:
x = w+4
@llvm.noalias(x, !foo);
*w <!foo> = *a <!foo>;

and now we have a problem, because it will look like *a and *w don't alias because they have the same scope tag but only one of them goes up through a pointer with the @llvm.noalias user.

To reiterate another rejected resign, we could make @llvm.noalias return a pointer, so we force it to be the (indirect) parent of all derived pointers. But we specifically don't want to do that because it will block exactly the kind of combining in the above example, and that is combining that we do want to happen. We also rejected any other design that introduces code-motion barriers around the inlined code.

So that leaves me with the original design (although, as I've mentioned, I need to fix the inlining attribute -> metadata procedure).

Hopefully I've not confused the issue too much; the benefit of having this small detour in writing is that in the future I'll remember why all of the rejected designs were rejected. ;)

So, here's my plan: I'm going to separate this into three patches:
 1. The basic refactoring.
 2. The scoped-noalias metadata
 3. The noalias parameter attribute -> metadata conversion
Since everyone was happy (including me) with (1) and (2), I'll commit those. I'll fix (3) and post it for review.

Thanks again everyone,
Hal

> 
> This is better because a) There is much less metadata b) it does not
> require a capturing analysis (BasicAA will do one at query time if
> necessary just as it does now). Because the metadata explicitly mark
> the applicable memory accesses, there is no need to derive that
> information from dominance, and thus no need to maintain control
> dependencies on the intrinsic itself (it can be marked IntNoMem,
> just like the debug intrinsics).
> 
> The good news is that any scheme we use (that does not involve
> scheduling barriers) must have some kind scope-tagging metadata, and
> so the refactoring of Loc.TBAATag -> Loc.AATags will be necessary
> regardless.
> 
> And here's the awkward part: Then C++ noalias attributes I've been
> working on (with lots of other folks), see
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n3988.pdf,
> aims to have the noalias set inheritance be syntactically derivable,
> and so probably prefers this current scheme (or some extension of
> it).
> 
> I'll update to the new scheme and re-post.
> 
> http://reviews.llvm.org/D2194
> 
> 
>

http://reviews.llvm.org/D2194