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

Paulo Matos via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jan 13 04:48:22 PST 2023


pmatos marked 15 inline comments as done.
pmatos added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:4129
+  let Documentation = [WebAssemblyExportNameDocs];
+  let Subjects = SubjectList<[TypedefName], ErrorDiag>;
+}
----------------
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 ?


================
Comment at: clang/include/clang/Basic/AttrDocs.td:6855
+Clang supports the ``__funcref`` attribute for the WebAssembly target. 
+This attribute may be attached to a function pointer declaration, where it modifies 
+its underlying representation to be a WebAssembly ``funcref``.
----------------
erichkeane wrote:
> This says 'function pointer declaration', but you have it's subject as Typedef?
That's a mistake due to how things worked in an earlier revision. Thanks for spotting. 


================
Comment at: clang/include/clang/Basic/TokenKinds.def:666
+// WebAssembly Type Extension
+KEYWORD(__funcref                     , KEYALL)
+
----------------
erichkeane wrote:
> aaron.ballman wrote:
> > pmatos wrote:
> > > aaron.ballman wrote:
> > > > Why is this a keyword in all language modes?
> > > I might have misread the docs but this keyword should be available for both C and C++. Maybe I want `KEYC99 | KEYCXX` ?
> > I was thinking this keyword would only work for a web assembly target, so we'd likely want to add `KEYWEBASM` or something to the list like we have for `KEYALTIVEC`. But now I'm second-guessing that instinct and am wondering what behavior we want here.
> > 
> > 1) Parse the keyword in all language modes and for all targets, give an ignored attribute warning if the target isn't web assembly
> > 2) Parse the keyword in all language modes and for all targets, give a parse time diagnostic (error?) if the target isn't web assembly
> > 3) Only expose the keyword if the target is web assembly, otherwise it parses as an identifier and you get the usual parse errors
> > 
> > My original thinking was that we wanted #3, but on reflection both #1 and #2 seem reasonable to me. Do you have a preference? I think I prefer either 2 or 3 over 1 because this involves the type system (errors are usually a better approach in that case).
> I don't think #1 is acceptable for the keyword, we shouldn't ignore keywords like we do attributes.  Doing an ignored warning in SemaDeclAttr.td for the ATTRIBUTE is fine, but keyword form needs to be an error.
> 
> My preference is #2, an error if we encounter it during parsing and we're not in WebAsm target.
Thanks for the comments, I have now updated this with an implementation of option 2 and added a diagnostic test.


================
Comment at: clang/lib/Basic/Targets/DirectX.h:45
     3, // hlsl_groupshared
+    // Wasm address space values for this map are dummy
+    20, // wasm_funcref
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128440



More information about the llvm-commits mailing list