[PATCH] D122215: [WebAssembly] Initial support for reference type externref in clang

[Paulo Matos via Phabricator via cfe-commits]

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


================
Comment at: clang/include/clang/Basic/AddressSpaces.h:59-60
 
+  wasm_var,
+  wasm_externref,
   // This denotes the count of language-specific address spaces and also
----------------
aaron.ballman wrote:
> tlively wrote:
> > What is `wasm_var`? It would be good to have a short comment here (and newline afterward).
> Agreed; I'm not certain why `wasm_var` is part of this changeset.
Yes, it was a mistake during patch splitting. I have removed it. Thanks.


================
Comment at: clang/lib/AST/ASTContext.cpp:2258
+    Width = 0;                                                                 \
+    Align = 8; /* ? */                                                         \
+    break;
----------------
tlively wrote:
> I assume things will break if you say 0 here, but would 1 work?
Yes, 1 seems to work. I am not 100% sure but since externref cannot be written to memory, does it matter?


================
Comment at: clang/lib/AST/ASTContext.cpp:3984
+QualType ASTContext::getExternrefType() const {
+  if (Target->hasFeature("reference-types")) {
+#define WASM_REF_TYPE(Name, MangledName, Id, SingletonId, AS)                  \
----------------
tlively wrote:
> Do we need `Target.getTriple().isWasm()` here as well?
Might be useful, in case another target has also a feature called `reference-types`. :)


================
Comment at: clang/lib/Basic/Targets/DirectX.h:44-45
+    0, // ptr64
+    1, // wasm_var
+    10,// wasm_externref
 };
----------------
tlively wrote:
> What are these for? I'm surprised we need to do anything here in the DirectX target. Same for the similar lines in other targets.
I was surprised as well, but apparently if you don't add them to all targets, they will not compile. There's an assignment of these Target specific AddrSpaceMaps to the AddrSpaceMap: `AddrSpaceMap = &DirectXAddrSpaceMap;`, and AddrSpaceMap is a LangAS defined in AddressSpaces.h so anything added there needs to be added to all targets afaiu. 


================
Comment at: clang/test/Sema/wasm-refs-and-tables.c:3
+
+// Note: As WebAssembly references are sizeless types, we don't exhaustively
+// test for cases covered by sizeless-1.c and similar tests.
----------------
tlively wrote:
> Should this file be just `wasm-refs.c` since it doesn't do anything with tables yet? Same for the next one.
Tables to be added in a future patch. ;)
But yeah, I will rename for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122215