[llvm] r317311 - [Analysis] Refine matching and merging of TBAA tags

Mikael Holmén via llvm-commits llvm-commits at lists.llvm.org
Tue Nov 7 01:10:31 PST 2017


Hi Ivan,

I've run into a problem when using your patch, could you please take a 
quick look at this and see if this is expected?

The problem that I get is that a load and a store are rescheduled so the 
load is done before the store, but it should be after.

Looking at what happens, I see that with your patch I get a difference 
in output from the load store vectorizer. The tbaa-metadata, !21, for 
the merged store is slightly different.

Before your change I get

!12 = !{!"omnipotent char", !13, i64 0}
!13 = !{!"Simple C/C++ TBAA"}
!21 = !{!12, !12, i64 0}

and with the patch I instead get

!12 = !{!"omnipotent char", !13, i64 0}
!13 = !{!"Simple C/C++ TBAA"}
!14 = !{!"short", !12, i64 0}
!21 = !{!14, !14, i64 0}

The two merged stores are a char and a short. On my target, both of them 
are i16 (we have a huge patch making llvm accept 16b bytes).

So, is the above difference in output intended? Is it correct?

I attached log printouts from LSV with before and after dumps. The whole 
module is dumped so metadata is shown as well.

Regards,
Mikael


On 11/03/2017 11:26 AM, Ivan A. Kosarev via llvm-commits wrote:
> Author: kosarev
> Date: Fri Nov  3 03:26:25 2017
> New Revision: 317311
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=317311&view=rev
> Log:
> [Analysis] Refine matching and merging of TBAA tags
> 
> This patch combines the code that matches and merges TBAA access
> tags. The aim is to simplify future changes and making sure that
> these operations produce consistent results.
> 
> Differential Revision: https://reviews.llvm.org/D39463
> 
> Modified:
>      llvm/trunk/lib/Analysis/TypeBasedAliasAnalysis.cpp
> 
> Modified: llvm/trunk/lib/Analysis/TypeBasedAliasAnalysis.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/TypeBasedAliasAnalysis.cpp?rev=317311&r1=317310&r2=317311&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Analysis/TypeBasedAliasAnalysis.cpp (original)
> +++ llvm/trunk/lib/Analysis/TypeBasedAliasAnalysis.cpp Fri Nov  3 03:26:25 2017
> @@ -314,17 +314,8 @@ AliasResult TypeBasedAAResult::alias(con
>     if (!EnableTBAA)
>       return AAResultBase::alias(LocA, LocB);
>   
> -  // Get the attached MDNodes. If either value lacks a tbaa MDNode, we must
> -  // be conservative.
> -  const MDNode *AM = LocA.AATags.TBAA;
> -  if (!AM)
> -    return AAResultBase::alias(LocA, LocB);
> -  const MDNode *BM = LocB.AATags.TBAA;
> -  if (!BM)
> -    return AAResultBase::alias(LocA, LocB);
> -
> -  // If they may alias, chain to the next AliasAnalysis.
> -  if (Aliases(AM, BM))
> +  // If accesses may alias, chain to the next AliasAnalysis.
> +  if (Aliases(LocA.AATags.TBAA, LocB.AATags.TBAA))
>       return AAResultBase::alias(LocA, LocB);
>   
>     // Otherwise return a definitive result.
> @@ -424,25 +415,24 @@ bool MDNode::isTBAAVtableAccess() const
>     return false;
>   }
>   
> +static bool matchAccessTags(const MDNode *A, const MDNode *B,
> +                            const MDNode **GenericTag = nullptr);
> +
>   MDNode *MDNode::getMostGenericTBAA(MDNode *A, MDNode *B) {
> +  const MDNode *GenericTag;
> +  matchAccessTags(A, B, &GenericTag);
> +  return const_cast<MDNode*>(GenericTag);
> +}
> +
> +static const MDNode *getLeastCommonType(const MDNode *A, const MDNode *B) {
>     if (!A || !B)
>       return nullptr;
>   
>     if (A == B)
>       return A;
>   
> -  // For struct-path aware TBAA, we use the access type of the tag.
> -  assert(isStructPathTBAA(A) && isStructPathTBAA(B) &&
> -         "Auto upgrade should have taken care of this!");
> -  A = cast_or_null<MDNode>(MutableTBAAStructTagNode(A).getAccessType());
> -  if (!A)
> -    return nullptr;
> -  B = cast_or_null<MDNode>(MutableTBAAStructTagNode(B).getAccessType());
> -  if (!B)
> -    return nullptr;
> -
> -  SmallSetVector<MDNode *, 4> PathA;
> -  MutableTBAANode TA(A);
> +  SmallSetVector<const MDNode *, 4> PathA;
> +  TBAANode TA(A);
>     while (TA.getNode()) {
>       if (PathA.count(TA.getNode()))
>         report_fatal_error("Cycle found in TBAA metadata.");
> @@ -450,8 +440,8 @@ MDNode *MDNode::getMostGenericTBAA(MDNod
>       TA = TA.getParent();
>     }
>   
> -  SmallSetVector<MDNode *, 4> PathB;
> -  MutableTBAANode TB(B);
> +  SmallSetVector<const MDNode *, 4> PathB;
> +  TBAANode TB(B);
>     while (TB.getNode()) {
>       if (PathB.count(TB.getNode()))
>         report_fatal_error("Cycle found in TBAA metadata.");
> @@ -462,7 +452,7 @@ MDNode *MDNode::getMostGenericTBAA(MDNod
>     int IA = PathA.size() - 1;
>     int IB = PathB.size() - 1;
>   
> -  MDNode *Ret = nullptr;
> +  const MDNode *Ret = nullptr;
>     while (IA >= 0 && IB >= 0) {
>       if (PathA[IA] == PathB[IB])
>         Ret = PathA[IA];
> @@ -472,17 +462,7 @@ MDNode *MDNode::getMostGenericTBAA(MDNod
>       --IB;
>     }
>   
> -  // We either did not find a match, or the only common base "type" is
> -  // the root node.  In either case, we don't have any useful TBAA
> -  // metadata to attach.
> -  if (!Ret || Ret->getNumOperands() < 2)
> -    return nullptr;
> -
> -  // We need to convert from a type node to a tag node.
> -  Type *Int64 = IntegerType::get(A->getContext(), 64);
> -  Metadata *Ops[3] = {Ret, Ret,
> -                      ConstantAsMetadata::get(ConstantInt::get(Int64, 0))};
> -  return MDNode::get(A->getContext(), Ops);
> +  return Ret;
>   }
>   
>   void Instruction::getAAMetadata(AAMDNodes &N, bool Merge) const {
> @@ -505,70 +485,107 @@ void Instruction::getAAMetadata(AAMDNode
>       N.NoAlias = getMetadata(LLVMContext::MD_noalias);
>   }
>   
> -/// Aliases - Test whether the type represented by A may alias the
> -/// type represented by B.
> -bool TypeBasedAAResult::Aliases(const MDNode *A, const MDNode *B) const {
> +static bool findAccessType(TBAAStructTagNode BaseTag,
> +                           const MDNode *AccessTypeNode,
> +                           uint64_t &OffsetInBase) {
> +  // Start from the base type, follow the edge with the correct offset in
> +  // the type DAG and adjust the offset until we reach the access type or
> +  // until we reach a root node.
> +  TBAAStructTypeNode BaseType(BaseTag.getBaseType());
> +  OffsetInBase = BaseTag.getOffset();
> +
> +  while (const MDNode *BaseTypeNode = BaseType.getNode()) {
> +    if (BaseTypeNode == AccessTypeNode)
> +      return true;
> +
> +    // Follow the edge with the correct offset, Offset will be adjusted to
> +    // be relative to the field type.
> +    BaseType = BaseType.getParent(OffsetInBase);
> +  }
> +  return false;
> +}
> +
> +static const MDNode *createAccessTag(const MDNode *AccessType) {
> +  Type *Int64 = IntegerType::get(AccessType->getContext(), 64);
> +  auto *ImmutabilityFlag = ConstantAsMetadata::get(ConstantInt::get(Int64, 0));
> +  Metadata *Ops[] = {const_cast<MDNode*>(AccessType),
> +                     const_cast<MDNode*>(AccessType), ImmutabilityFlag};
> +  return MDNode::get(AccessType->getContext(), Ops);
> +}
> +
> +/// matchTags - Return true if the given couple of accesses are allowed to
> +/// overlap. If \arg GenericTag is not null, then on return it points to the
> +/// most generic access descriptor for the given two.
> +static bool matchAccessTags(const MDNode *A, const MDNode *B,
> +                            const MDNode **GenericTag) {
> +  if (A == B) {
> +    if (GenericTag)
> +      *GenericTag = A;
> +    return true;
> +  }
> +
> +  // Accesses with no TBAA information may alias with any other accesses.
> +  if (!A || !B) {
> +    if (GenericTag)
> +      *GenericTag = nullptr;
> +    return true;
> +  }
> +
>     // Verify that both input nodes are struct-path aware.  Auto-upgrade should
>     // have taken care of this.
> -  assert(isStructPathTBAA(A) && "MDNode A is not struct-path aware.");
> -  assert(isStructPathTBAA(B) && "MDNode B is not struct-path aware.");
> +  assert(isStructPathTBAA(A) && "Access A is not struct-path aware!");
> +  assert(isStructPathTBAA(B) && "Access B is not struct-path aware!");
>   
> -  // Keep track of the root node for A and B.
> -  TBAAStructTypeNode RootA, RootB;
>     TBAAStructTagNode TagA(A), TagB(B);
>   
>     // TODO: We need to check if AccessType of TagA encloses AccessType of
>     // TagB to support aggregate AccessType. If yes, return true.
>   
> -  // Start from the base type of A, follow the edge with the correct offset in
> -  // the type DAG and adjust the offset until we reach the base type of B or
> -  // until we reach the Root node.
> -  // Compare the adjusted offset once we have the same base.
> -
> -  // Climb the type DAG from base type of A to see if we reach base type of B.
>     const MDNode *BaseA = TagA.getBaseType();
>     const MDNode *BaseB = TagB.getBaseType();
> -  uint64_t OffsetA = TagA.getOffset(), OffsetB = TagB.getOffset();
> -  for (TBAAStructTypeNode T(BaseA);;) {
> -    if (T.getNode() == BaseB)
> -      // Base type of A encloses base type of B, check if the offsets match.
> -      return OffsetA == OffsetB;
>   
> -    RootA = T;
> -    // Follow the edge with the correct offset, OffsetA will be adjusted to
> -    // be relative to the field type.
> -    T = T.getParent(OffsetA);
> -    if (!T.getNode())
> -      break;
> +  // Climb the type DAG from base type of A to see if we reach base type of B.
> +  uint64_t OffsetA;
> +  if (findAccessType(TagA, BaseB, OffsetA)) {
> +    if (GenericTag)
> +      *GenericTag = createAccessTag(TagB.getAccessType());
> +    return OffsetA == TagB.getOffset();
>     }
>   
> -  // Reset OffsetA and climb the type DAG from base type of B to see if we reach
> -  // base type of A.
> -  OffsetA = TagA.getOffset();
> -  for (TBAAStructTypeNode T(BaseB);;) {
> -    if (T.getNode() == BaseA)
> -      // Base type of B encloses base type of A, check if the offsets match.
> -      return OffsetA == OffsetB;
> -
> -    RootB = T;
> -    // Follow the edge with the correct offset, OffsetB will be adjusted to
> -    // be relative to the field type.
> -    T = T.getParent(OffsetB);
> -    if (!T.getNode())
> -      break;
> +  // Climb the type DAG from base type of B to see if we reach base type of A.
> +  uint64_t OffsetB;
> +  if (findAccessType(TagB, BaseA, OffsetB)) {
> +    if (GenericTag)
> +      *GenericTag = createAccessTag(TagA.getAccessType());
> +    return OffsetB == TagA.getOffset();
>     }
>   
> -  // Neither node is an ancestor of the other.
> +  // If neither node is an ancestor of the other, then try to find the type
> +  // that is common to both the final access types.
> +  const MDNode *CommonType = getLeastCommonType(TagA.getAccessType(),
> +                                                TagB.getAccessType());
> +
> +  // If there is no common type or the only common type is the root node, then
> +  // we don't have any useful generic access tag to return.
> +  if (GenericTag)
> +    *GenericTag = !CommonType || CommonType->getNumOperands() < 2 ?
> +        nullptr : createAccessTag(CommonType);
>   
>     // If they have different roots, they're part of different potentially
>     // unrelated type systems, so we must be conservative.
> -  if (RootA.getNode() != RootB.getNode())
> +  if (!CommonType)
>       return true;
>   
>     // If they have the same root, then we've proved there's no alias.
>     return false;
>   }
>   
> +/// Aliases - Test whether the access represented by tag A may alias the
> +/// access represented by tag B.
> +bool TypeBasedAAResult::Aliases(const MDNode *A, const MDNode *B) const {
> +  return matchAccessTags(A, B);
> +}
> +
>   AnalysisKey TypeBasedAA::Key;
>   
>   TypeBasedAAResult TypeBasedAA::run(Function &F, FunctionAnalysisManager &AM) {
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lsv.log
Type: text/x-log
Size: 10850 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171107/daa850b5/attachment.bin>


More information about the llvm-commits mailing list