Page MenuHomePhabricator

[Metadata] Add TBAA struct metadata to `AAMDNode`
ClosedPublic

Authored by anton-afanasyev on Dec 2 2019, 2:05 PM.

Details

Summary

Make AAMDNodes' getAAMetadata() and setAAMetadata() to take !tbaa.struct
into account like for !tbaa. This impacts llvm.org/pr42022.
This is a temprorary fix needed to keep !tbaa.struct tag by SROA pass. New field TBAAStruct should be deleted when !tbaa tag replaces !tbaa.struct. Merging two !tbaa.struct's to one is conservatively considered to be nullptr (giving MayAlias) -- this could be enhanced, but relying on the said replacement.

Diff Detail

Event Timeline

anton-afanasyev created this revision.Dec 2 2019, 2:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2019, 2:05 PM
anton-afanasyev edited the summary of this revision. (Show Details)Dec 3 2019, 7:53 AM
anton-afanasyev added a reviewer: ABataev.
hfinkel added inline comments.Dec 6 2019, 6:34 AM
llvm/lib/Analysis/TypeBasedAliasAnalysis.cpp
527

Does MDNode::getMostGenericTBAA actually support struct.tbaa metadata? I didn't think that it did.

anton-afanasyev marked an inline comment as done.Dec 9 2019, 2:24 AM
anton-afanasyev added inline comments.
llvm/lib/Analysis/TypeBasedAliasAnalysis.cpp
527

No, it really didn't. Fixing it.

Hi @hfinkel, I've investigated the current state of TBAA support in LLVM. AFAIU (see D40975, but it is still uncommited) the new format of tbaa tag replaces tbaa.struct tag. Does it mean tbaa.struct tag will be deprecated soon?

Updated getAAMetadata() with Merge=true

anton-afanasyev marked an inline comment as done.Dec 11 2019, 10:33 AM
anton-afanasyev added inline comments.
llvm/lib/Analysis/TypeBasedAliasAnalysis.cpp
527

Updated. I conservatively suppose that merging two !tbaa.struct's turns to MayAlias.

Hi @hfinkel, I've investigated the current state of TBAA support in LLVM. AFAIU (see D40975, but it is still uncommited) the new format of tbaa tag replaces tbaa.struct tag. Does it mean tbaa.struct tag will be deprecated soon?

This replacing !tbaa.struct->!tbaa is described at the topic http://lists.llvm.org/pipermail/llvm-dev/2017-October/118284.html started by @kosarev, and I see it has already been supported and almost commited (but not announced). So my patch looks like temprorary bug fixing until this transfer is finished.

Hi @hfinkel, I've investigated the current state of TBAA support in LLVM. AFAIU (see D40975, but it is still uncommited) the new format of tbaa tag replaces tbaa.struct tag. Does it mean tbaa.struct tag will be deprecated soon?

It might, but it depends on the progress on the new TBAA format. This is not a large change, and I think that we can proceed with this regardless.

Can you please explain what will end up using this information? That should be present in the description.

anton-afanasyev edited the summary of this revision. (Show Details)Dec 12 2019, 12:19 PM

Hi @hfinkel, I've investigated the current state of TBAA support in LLVM. AFAIU (see D40975, but it is still uncommited) the new format of tbaa tag replaces tbaa.struct tag. Does it mean tbaa.struct tag will be deprecated soon?

It might, but it depends on the progress on the new TBAA format. This is not a large change, and I think that we can proceed with this regardless.

Can you please explain what will end up using this information? That should be present in the description.

Ok, updated summary.

fhahn added a subscriber: fhahn.Dec 12 2019, 1:37 PM

Gently ping. Is it ok now?

Gently ping again.

RKSimon added inline comments.Jan 3 2020, 5:54 AM
llvm/include/llvm/IR/Metadata.h
647

I always get worried when constructors with default args get rearranged like this. Maybe change to this:

explicit AAMDNodes() = default;
explicit AAMDNodes(MDNode *T, MDNode *TS, MDNode *S, MDNode *N)
    : TBAA(T), TBAAStruct(TS), Scope(S), NoAlias(N) {}

MDNode *TBAA = nullptr; // etc.
anton-afanasyev marked an inline comment as done.

Update AAMDNodes constructors

anton-afanasyev marked an inline comment as done.Jan 3 2020, 11:25 AM
anton-afanasyev added inline comments.
llvm/include/llvm/IR/Metadata.h
647

Ok, done.

@hfinkel Any more comments?

hfinkel accepted this revision.Jan 5 2020, 3:15 PM

@hfinkel Any more comments?

Nope. LGTM.

This revision is now accepted and ready to land.Jan 5 2020, 3:15 PM
This revision was automatically updated to reflect the committed changes.