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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to