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