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

[Thomas Lively via Phabricator via cfe-commits]

tlively added a comment.

Very nice! This LGTM with all these small comments addressed. Sorry for the long delay in reviewing.



================
Comment at: clang/include/clang/AST/ASTContext.h:1149
 #include "clang/Basic/RISCVVTypes.def"
+#define WASM_TYPE(Name, Id, SingletonId) CanQualType SingletonId;
+#include "clang/Basic/WebAssemblyReferenceTypes.def"
----------------
I have no idea what's going on here in the code, but it seems that the existing convention is to put `CanQualType SingletonId;` on a separate line.


================
Comment at: clang/include/clang/AST/Type.h:1973
+  bool isWebAssemblyReferenceType() const;
+  bool isWebAssemblyExternrefType() const;
   /// Determines if this is a sizeless type supported by the
----------------
Looks like there should be a newline here.


================
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
----------------
What is `wasm_var`? It would be good to have a short comment here (and newline afterward).


================
Comment at: clang/include/clang/Basic/Builtins.def:50
 //  p -> pid_t
+//  e -> wasm externref
 //  . -> "...".  This may only occur at the end of the function list.
----------------



================
Comment at: clang/include/clang/Basic/BuiltinsWebAssembly.def:193
+// Reference Types builtins
+// Some builtins are polymorphic - see 't' as part of the third argument,
+// in which case the argument spec (second argument) is unused.
----------------
Looks like this comment about polymorphism is out of date. (Also it would be good to add a newline after this.)


================
Comment at: clang/include/clang/Basic/WebAssemblyReferenceTypes.def:9
+//
+//  This file defines externref_t, funcref_t, and the like.  The macros are:
+//
----------------
Maybe we should only mention `externref_t` for now. 


================
Comment at: clang/lib/AST/ASTContext.cpp:2258
+    Width = 0;                                                                 \
+    Align = 8; /* ? */                                                         \
+    break;
----------------
I assume things will break if you say 0 here, but would 1 work?


================
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)                  \
----------------
Do we need `Target.getTriple().isWasm()` here as well?


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:3147
+    type_name = MangledName;                                                   \
+    Out << (type_name == InternalName ? "u" : "") << type_name.size()          \
+        << type_name;                                                          \
----------------
Our `MangledName` is not the same as our `InternalName`, so it looks like this condition will never be true. Should be follow the simpler pattern from the previous two targets instead?


================
Comment at: clang/lib/Basic/Targets/DirectX.h:44-45
+    0, // ptr64
+    1, // wasm_var
+    10,// wasm_externref
 };
----------------
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.


================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:806-807
+      SingletonId =                                                            \
+          DBuilder.createForwardDecl(llvm::dwarf::DW_TAG_structure_type,       \
+                                     MangledName, TheCU, TheCU->getFile(), 0); \
+    return SingletonId;                                                        \
----------------
How did you choose this?


================
Comment at: clang/lib/CodeGen/TargetInfo.h:353
+  /// Return the WebAssembly externref reference type.
+  virtual llvm::Type *getWasmExternrefReferenceType() const { return nullptr; }
   /// Emit the device-side copy of the builtin surface type.
----------------
missing whitespace.


================
Comment at: clang/test/CodeGen/WebAssembly/wasm-externref.c:11-13
+externref_t get_null() {
+  return __builtin_wasm_ref_null_extern();
+}
----------------
Do we need this here since the builtin is also tested in builtins-wasm.c? Are there more ways to use `externref_t` that we should test here?


================
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.
----------------
Should this file be just `wasm-refs.c` since it doesn't do anything with tables yet? Same for the next one.


================
Comment at: clang/test/SemaTemplate/address_space-dependent.cpp:46
 void tooBig() {
-  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388588)}}
+  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388586)}}
 }
----------------
Maybe this test can be split off into a separate tiny PR since it doesn't look directly related to the rest.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122215