[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added reviewers: rjmccall, rsmith, erichkeane.
aaron.ballman added a comment.

In D120296#3343673 <https://reviews.llvm.org/D120296#3343673>, @yonghong-song wrote:

> @aaron.ballman  ping, did you get time to look at this patch?

Sorry, I was in C meetings all last week, still digging out from under the emails that piled up.

I think the approach taken here is likely incorrect. Not all `AttributedType` objects need this "extra info" as a string, and there's no reason to believe future attributes will want their extra info to be in the form of a string. However, there's also a fundamental flaw -- the `AttributedType` isn't tracking that extra information, so it cannot be profiled in all circumstances (I left a comment in the review for that).

As best I can tell, you want a different attribute argument to result in a different type in the type system. That needs to happen by adding a new type to the type system instead of using the generic `AttributedType` type. This new type can track the extra information needed for uniquing the type within the type system. As an example, `VectorType` is formed via an attribute, but it needs additional information about what kind of vector it is. I don't think you can do what you're trying in this patch because the `AttributedType` tracks the attribute *kind* but not the actual semantic attribute in use. You need an `AttributedTypeLoc` to get access to that via the type system, and you don't always have access to type locations.



================
Comment at: clang/include/clang/AST/Type.h:4774
   void Profile(llvm::FoldingSetNodeID &ID) {
     Profile(ID, getAttrKind(), ModifiedType, EquivalentType);
   }
----------------
This form of type profiling was not updated (and can't be, because there's nothing tracking what extra info is associated with the type), which further suggests we need a new type in the type system.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120296/new/

https://reviews.llvm.org/D120296