[PATCH] D128440: [WebAssembly] Initial support for reference type funcref in clang

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:4129
+  let Documentation = [WebAssemblyExportNameDocs];
+  let Subjects = SubjectList<[TypedefName], ErrorDiag>;
+}
----------------
pmatos wrote:
> erichkeane wrote:
> > Note that it seems this is likely not going to work correctly on template type aliases!  You probably want to make sure that is acceptable to you.
> Documentation says about Subject:
> 
> ```
>   // The things to which an attribute can appertain
>   SubjectList Subjects;
> ```
> 
> Given this can be attached to function pointer types, is the subject then just Type ?
IIRC, the list of subjects are the Decls and Stmt types that are possibly allowed.  I don't thnk it would be just 'Type' there.

As you have it, it is only allowed on TypedefNameDecl.

See the SubsetSubject's at the top of this file for some examples on how you could constrain a special matcher to only work on function pointer decls.


================
Comment at: clang/lib/Basic/Targets/DirectX.h:45
     3, // hlsl_groupshared
+    // Wasm address space values for this map are dummy
+    20, // wasm_funcref
----------------
pmatos wrote:
> erichkeane wrote:
> > Also apply to the rest of the targets.  Also, why is there only 1 added here?  I thought we had 2 address spaces for Wasm?
> Externref is a type in clang, doesn't need its own address space in clang, only when it's lowered to a opaque type in llvm it gets its own address space.
Please fix the comment in my suggestion.

Also, your explanation doesn't make it clear to me why you added TWO entries to this list for other targets, but only 1 here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128440