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

[Erich Keane via Phabricator via llvm-commits]

erichkeane added inline comments.


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


================
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``.
----------------
This says 'function pointer declaration', but you have it's subject as Typedef?


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


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


================
Comment at: clang/lib/Basic/Targets/WebAssembly.h:44-45
+    0,  // ptr64
+    10, // wasm_externref,
+    20, // wasm_funcref
+};
----------------
aaron.ballman wrote:
> 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?
That DEFINITELY looks suspicious to me as well, I noted above too, but either we need to specify we're re-using the `hlsl_groupshared` for `wasm_externref` address space (at which point, i wonder: why not re-use a different address space?), or correct this.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:847
+    SourceLocation AttrNameLoc = ConsumeToken();
+    attrs.addNew(AttrName, AttrNameLoc, nullptr, AttrNameLoc, nullptr, 0,
+                 ParsedAttr::AS_Keyword);
----------------



================
Comment at: clang/lib/Sema/SemaChecking.cpp:6696
+bool Sema::BuiltinWasmRefNullFunc(CallExpr *TheCall) {
+  if (TheCall->getNumArgs() != 0)
+    return true;
----------------
Does this diagnose?  Should we be emitting an error here?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:6708
+  // CallExpr
+  // `- FunctionDecl
+
----------------
All those comments don't really seem relevant here, and are not really correct.  The only thing SemaChecking should do is either modify its arguments (though you don't have any), or set its return type.  


================
Comment at: clang/lib/Sema/SemaChecking.cpp:6711
+  // Prepare FDecl type
+  QualType Pointee = Context.getFunctionType(Context.VoidTy, {}, {});
+  QualType Type = Context.getPointerType(Pointee);
----------------
aaron.ballman wrote:
> 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.
This appears to be a 'custom type checking' builtin, where the code in Sema has to check the arguments (rather than the automatic behavior working), and set its return type to the 'correct' one.  It appears that this accepts no arguments, then changes the result of the builtin.

So this doesn't seem particularly odd, other than the strange comments above.


================
Comment at: clang/lib/Sema/SemaType.cpp:7340
+  attr::Kind NewAttrKind = A->getKind();
+  QualType Desugared = Type;
+  const auto *AT = dyn_cast<AttributedType>(Type);
----------------
This name seems misleading... should you be desugaring 'Type' to store it in 'Desugared'?

Also, I see that this value is never used, so why do the copy init?  ALSO, why is it called Desugared, when it never seems to get a Desugared type?  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128440