[PATCH] Scoped NoAlias Metadata

[hfinkel at anl.gov]

Andy,

Thanks for the review! I apologize for taking so long to get back to this.

I've attached a revised patch, implementing your suggestions, rebasing, and including a LangRef addition. Unfortunately, I've hit a file upload limit again on phab with the full-context diff (which I've e-mailed Manuel about, but as I recall, he said he's on vacation for two weeks).

 -Hal

----- Original Message -----
> From: "Andrew Trick" <atrick at apple.com>
> To: atrick at apple.com, sunfish at google.com, chandlerc at gmail.com, hfinkel at anl.gov
> Cc: llvm-commits at cs.uiuc.edu, nrotem at apple.com, "renato golin" <renato.golin at linaro.org>, nlewycky at google.com
> Sent: Saturday, January 11, 2014 1:12:32 AM
> Subject: Re: [PATCH] Scoped NoAlias Metadata
> 
> 
>   Hal, this looks pretty good!
> 
>   On the design, we could do better that a quadratic search on each
>   query over two lists of AA scopes. But it doesn't seem worth the
>   complexity at this time.
> 
> 
> ================
> Comment at: lib/Analysis/ScopedNoAliasAA.cpp:82-85
> @@ +81,6 @@
> +
> +namespace {
> +  /// ScopedNoAliasAA - This is a simple alias analysis
> +  /// implementation that uses scoped-noalias metadata to answer
> queries.
> +  class ScopedNoAliasAA : public ImmutablePass, public AliasAnalysis
> {
> +  public:
> ----------------
> I personally don't like indenting large class definitions within
> anonymous namespace. It's unnecessary indentation and I find myself
> wondering if I'm looking at a nested class.
> 
> ================
> Comment at: lib/Analysis/ScopedNoAliasAA.cpp:136-141
> @@ +135,8 @@
> +
> +/// Aliases - Test whether the scope represented by A may alias the
> +/// scope represented by B. Specifically, A is the target scope, and
> B is the
> +/// noalias scope.
> +bool
> +ScopedNoAliasAA::Aliases(const MDNode *A,
> +                         const MDNode *B) const {
> +  // Climb the tree from A to see if we reach B.
> ----------------
> I know this is consistent with TBAA, but I find it misleading:
> Aliases() returns true on an unknown relation. Shouldn't it be
> called MayAlias()?
> 
> ================
> Comment at: lib/Analysis/ScopedNoAliasAA.cpp:168-174
> @@ +167,9 @@
> +
> +  if (AScopes && BNoAlias)
> +    for (unsigned i = 0, ie = AScopes->getNumOperands(); i != ie;
> ++i)
> +      if (const MDNode *AMD =
> dyn_cast<MDNode>(AScopes->getOperand(i)))
> +        for (unsigned j = 0, je = BNoAlias->getNumOperands(); j !=
> je; ++j)
> +          if (const MDNode *BMD =
> dyn_cast<MDNode>(BNoAlias->getOperand(j)))
> +            if (!Aliases(AMD, BMD))
> +              return NoAlias;
> +
> ----------------
> Please put this in a helper instead of repeating it four times:
> 
> e.g.
> 
> +static bool mayAliasInScopes(const MDNode *Scopes, const MDNode
> *NoAlias) {
> +  if (!Scopes || !NoAlias)
> +    return false;
> +  for (unsigned i = 0, ie = Scopes->getNumOperands(); i != ie; ++i)
> {
> +    if (const MDNode *SMD = dyn_cast<MDNode>(Scopes->getOperand(i)))
> +      for (unsigned j = 0, je = NoAlias->getNumOperands(); j != je;
> ++j)
> +        if (const MDNode *NAMD =
> dyn_cast<MDNode>(NoAlias->getOperand(j)))
> +          if (!MayAlias(SMD, NAMD))
> +            return false;
> +  }
> +  return true;
> 
> 
> ================
> Comment at: test/Transforms/Inline/noalias2.ll:43
> @@ +42,3 @@
> +
> +; Check that when hello() in inlined into foo(), and then foo() is
> inlined into
> +; foo2(), the noalias scopes are properly concatenated.
> ----------------
> is inlined
> 
> 
> http://llvm-reviews.chandlerc.com/D2194
>

- {F93633, layout=link}
- {F93632, layout=link}

http://reviews.llvm.org/D2194