[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 03:53:46 PST 2017
Hi,
On 11/07/2017 11:23 AM, Ivan Kosarev wrote:
> 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.
>
Thanks for the quick reply!
> If you have a reproducer that works for the mainline, will you please
> file a bug and attach it there?
>
Yes, I think I managed to create an example that exposes this also on
x86. I wrote PR35225 about it:
https://bugs.llvm.org/show_bug.cgi?id=35225
Thanks,
Mikael
> 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