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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
