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

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added a reviewer: erichkeane.
aaron.ballman added a subscriber: erichkeane.
aaron.ballman added a comment.

In D128440#4042753 <https://reviews.llvm.org/D128440#4042753>, @pmatos wrote:

> @aaron.ballman Hi Aaron, since you requested an RFC for this. I wonder if you have any comments for this or the other related patches: D122215 <https://reviews.llvm.org/D122215>, D139010 <https://reviews.llvm.org/D139010>

I appreciate your patience with the extreme delay on getting you feedback after I asked you to post the RFC; I wanted to give the RFC a chance to bake and then ran into the holiday season as a result. Sorry for that!

Adding @erichkeane as new attributes code owner as well, in case he's got additional thoughts.



================
Comment at: clang/include/clang/Basic/TokenKinds.def:666
+// WebAssembly Type Extension
+KEYWORD(__funcref                     , KEYALL)
+
----------------
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).


================
Comment at: clang/lib/AST/TypePrinter.cpp:1652-1653
 
+  if (T->isWebAssemblyFuncrefSpec()) {
+    assert(T->getAttrKind() == attr::WebAssemblyFuncref);
+    OS << "__funcref";
----------------
I'm not certain that the `assert` adds much here given that the predicate tests exactly the same thing, perhaps remove it (and drop the curly braces)?


================
Comment at: clang/lib/Basic/Targets/WebAssembly.h:44-45
+    0,  // ptr64
+    10, // wasm_externref,
+    20, // wasm_funcref
+};
----------------
I'm not super familiar with the address space maps, but this sets of my spidey senses. All of the other address space maps end with:

ptr64, hlsl_groupshared, wasm_funcref

but this one is ending with:

ptr64, wasm_externref, wasm_funcref

which makes me think something's amiss here. Thoughts?


================
Comment at: clang/lib/Parse/ParseDecl.cpp:844
+void Parser::ParseWebAssemblyFuncrefTypeAttribute(ParsedAttributes &attrs) {
+  if (Tok.is(tok::kw___funcref)) {
+    IdentifierInfo *AttrName = Tok.getIdentifierInfo();
----------------
I think we should assert this condition; we already know what the token is before we call this function anyway.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:5911-5914
+      if (AttrReqs & AR_DeclspecAttributesParsed) {
+        ParseWebAssemblyFuncrefTypeAttribute(DS.getAttributes());
+        continue;
+      }
----------------
I don't think we need to care about attribute requirements because this is a keyword attribute (this feature is used to prohibit attributes in certain grammars and I don't think that applies here).


================
Comment at: clang/lib/Sema/SemaChecking.cpp:6704-6710
+  // Therefore we need to change the types of the DeclRefExpr (stored in FDecl)
+  // and regenerate a straight up CallExpr on the modified FDecl.
+  // returning
+  // CallExpr
+  // `- FunctionDecl
+
+  // Prepare FDecl type
----------------
These comments no longer seem to match the code -- nothing creates a `FunctionDecl` here.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:6711
+  // Prepare FDecl type
+  QualType Pointee = Context.getFunctionType(Context.VoidTy, {}, {});
+  QualType Type = Context.getPointerType(Pointee);
----------------
I'm still wondering why this code is needed and what it's trying to do. It worries me that we're changing the type of the call expression like this.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:14816
+      // WebAssembly allows reference types as parameters. Funcref in particular
+      // lives in a different address space
+      !(T->isFunctionPointerType() &&
----------------



================
Comment at: clang/lib/Sema/SemaType.cpp:7351
+  if (Attrs[NewAttrKind]) {
+    S.Diag(PAttr.getLoc(), diag::warn_duplicate_attribute_exact) << PAttr;
+    return true;
----------------
We're missing Sema test coverage for all the various diagnostics you've added; can you add that coverage?


================
Comment at: clang/lib/Sema/SemaType.cpp:7269
+  }
+  Attrs[NewAttrKind] = true;
+
----------------
aaron.ballman wrote:
> This seems like it's not used.
Still wondering about this


================
Comment at: clang/lib/Sema/SemaType.cpp:7289-7290
+  QualType Pointee = Type->getPointeeType();
+  Pointee = S.Context.getAddrSpaceQualType(
+      S.Context.removeAddrSpaceQualType(Pointee), ASIdx);
+  Type = State.getAttributedType(A, Type, S.Context.getPointerType(Pointee));
----------------
aaron.ballman wrote:
> What happens when the user's function already has an explicitly specified address space?
Still wondering about this as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128440