[PATCH] D139010: [clang][WebAssembly] Implement support for table types and builtins

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/AST/ASTContext.h:1511
+  /// element type
+  QualType getWasmTableType(QualType EltTy) const;
+
----------------
Ah, I suggested `WebAssembly` in another review, but `Wasm` is also fine by me -- just pick a consistent term to use.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2171-2176
 def err_reference_bind_to_vector_element : Error<
   "%select{non-const|volatile}0 reference cannot bind to vector element">;
 def err_reference_bind_to_matrix_element : Error<
   "%select{non-const|volatile}0 reference cannot bind to matrix element">;
+def err_reference_bind_to_table_element : Error<
+  "%select{non-const|volatile}0 reference cannot bind to table element">;
----------------
We should combine these diagnostics with a `%select` rather than keep duplicating the same message.

This is unused and untested.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3035-3040
 def err_attribute_invalid_vector_type : Error<"invalid vector element type %0">;
 def err_attribute_invalid_bitint_vector_type : Error<
   "'_BitInt' vector element width must be %select{a power of 2|"
   "at least as wide as 'CHAR_BIT'}0">;
 def err_attribute_invalid_matrix_type : Error<"invalid matrix element type %0">;
+def err_attribute_invalid_wasm_table_type : Error<"invalid table element type %0">;
----------------
We should probably combine these as well.

This is unused and untested.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6930
 def err_typecheck_address_of : Error<"address of %select{bit-field"
-  "|vector element|property expression|register variable|matrix element}0 requested">;
+  "|vector element|property expression|register variable|matrix element|table element}0 requested">;
 def ext_typecheck_addrof_void : Extension<
----------------
This is untested.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11638
+def err_wasm_table_disabled: Error<
+  "WebAssembly table support is disabled. Pass -mreference-types to enable it">;
 def err_builtin_matrix_disabled: Error<
----------------
I know you copied the text from the matrix diagnostic, but that diagnostic does not follow our usual style rules.

Why an `-m` option instead of a `-f` option?


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11641-11642
   "matrix types extension is disabled. Pass -fenable-matrix to enable it">;
+def err_table_index_not_integer: Error<
+  "table index must be an integer">;
 def err_matrix_index_not_integer: Error<
----------------
This is unused and untested.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11792-11795
+def err_wasm_table_pointer : Error<
+  "pointers to WebAssembly tables are illegal">;
+def err_wasm_table_reference : Error<
+  "references to WebAssembly tables are illegal">;
----------------
You can combine these and reword to `cannot form a %select{pointer|reference}0 to a WebAssembly table`


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11794
+  "pointers to WebAssembly tables are illegal">;
+def err_wasm_table_reference : Error<
+  "references to WebAssembly tables are illegal">;
----------------
This is untested. I'll stop commenting on those at this point, please ensure you're testing all diagnostics you've added.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11797
+def err_typecheck_wasm_table_must_have_zero_length : Error<
+  "only zero-length WebAssembly tables are currently supported">;
+def err_wasm_table_in_function : Error<
----------------
Do you plan to support non-zero-length tables in the near future? I'm wondering if this should be reworded to remove the "currently supported" phrasing.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11800-11801
+  "WebAssembly table cannot be declared within a function">;
+def err_wasm_table_as_function_argument : Error<
+  "cannot use WebAssembly table as a function argument">;
+def err_wasm_table_invalid_uett_operand : Error<
----------------



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11805
+def err_wasm_cast_table : Error<
+  "cannot cast WebAssembly table">;
+def err_wasm_table_conditional_expression : Error<
----------------



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11807
+def err_wasm_table_conditional_expression : Error<
+  "cannot use WebAssembly tables as the 2nd or 3rd operands of a conditional expression">;
+def err_wasm_table_assignment : Error<
----------------
You should also add test coverage for the weird GNU binary conditional operator: https://gcc.gnu.org/onlinedocs/gcc/Conditionals.html (I reworded the diagnostic to make that less awkward).


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11810-11815
+def err_wasm_table_return : Error<
+  "cannot return a WebAssembly table">;
+def err_wasm_table_throw : Error<
+  "cannot throw a WebAssembly table">;
+def err_wasm_table_catch : Error<
+  "cannot catch WebAssembly table">;
----------------
Combine


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11827
+def err_wasm_builtin_arg_must_be_integer_type : Error <
+  "%ordinal0 argument must be an integer">;
 } // end of sema component.
----------------
I'm a bit shocked by the number of new diagnostics for this type as it seems incredibly restrictive and like the rules are going to be hard to understand. For example, you cannot use this type in an exception specification despite that being a compile-time property. Can you use it within a conditional explicit clause (https://godbolt.org/z/sn3G8xE3T)? It must be static, but can it be thread local?

Basically, it seems like this type is unlike basically any other type and we're going to have to carry a significant amount of extra code around to handle all the edge cases and those edge cases look a bit like whack-a-mole in practice.


================
Comment at: clang/lib/AST/Type.cpp:2355-2360
+  if (const auto *ATy = dyn_cast<ArrayType>(this)) {
+    return ATy->getElementType().isWebAssemblyReferenceType();
+  }
+  if (const auto *PTy = dyn_cast<PointerType>(this)) {
+    return PTy->getPointeeType().isWebAssemblyReferenceType();
+  }
----------------



================
Comment at: llvm/include/llvm/CodeGen/WasmAddressSpaces.h:44
+} // namespace llvm
\ No newline at end of file

----------------
Add newline


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139010