[llvm] r317311 - [Analysis] Refine matching and merging of TBAA tags
Ivan Kosarev via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 7 02:23:44 PST 2017
Hello Mikael,
Merging a short access with a char access certainly shouldn't result in
a short access, so it is not intended. I suspect the problem is that we
assume that all mergeable accesses to members of the same structure (or
vector) type have the same final access types. In your case we access
differently typed members of the same aggregate, hence the wrong result.
Will address this ASAP.
If you have a reproducer that works for the mainline, will you please
file a bug and attach it there?
Thanks for reporting and sorry for the troubles,
On 07/11/17 11:10, Mikael Holmén wrote:
> 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
>>
More information about the llvm-commits
mailing list