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

[Paulo Matos via Phabricator via llvm-commits]

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


================
Comment at: clang/include/clang/AST/Stmt.h:456
 
-  class ArrayOrMatrixSubscriptExprBitfields {
+  class AMTSubscriptExprBitfields {
     friend class ArraySubscriptExpr;
----------------
aaron.ballman wrote:
> I am not certain what `AMT` stands for -- you might need some comments to explain what this stands for.
Ah yes,  somehow I missed this. I renamed this at some point with an application in mind and then didn't use it. So renamed back.

AMT was supposed to be Array / Matrix / Table.


================
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.
----------------
aaron.ballman wrote:
> pmatos wrote:
> > asb wrote:
> > > aaron.ballman wrote:
> > > > 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.
> > > The whack-a-mole aspect of disallowing table uses is something I'm not fond of either....but I'm not sure I see a better approach. Do you have any alternatives in mind?
> > A couple concrete answers in addition to what @asb already said.
> > 
> > * I don't think we can use tables at the moment in conditional explicit clause. Indeed it doesn't make sense since there's no what, at the moment, to statically initialize the table.
> > * Tables are thread local. Indeed, threads in WebAssembly share only linear memory so anything that's not in linear memory is thread local. This is true for tables but also for all reference types.
> > The whack-a-mole aspect of disallowing table uses is something I'm not fond of either....but I'm not sure I see a better approach. Do you have any alternatives in mind?
> 
> I don't have good ideas off the top of my head, but this is a lot of overhead for the feature so my default idea is "don't add this type to the compiler" which may not help all that much. Do you have evidence that this type is necessary, will have enough uses in the wild to justify adding it, and is not possible to make it more regular?
> > The whack-a-mole aspect of disallowing table uses is something I'm not fond of either....but I'm not sure I see a better approach. Do you have any alternatives in mind?
> 
> I don't have good ideas off the top of my head, but this is a lot of overhead for the feature so my default idea is "don't add this type to the compiler" which may not help all that much. Do you have evidence that this type is necessary, will have enough uses in the wild to justify adding it, and is not possible to make it more regular?

A table is a container for reference types - i.e. externref, funcref and others in the future and an essential part of WebAssembly. I don't see how we can compile down to tables unless we enforce these constraints at this level. I wish there was a way of providing the same functionality without needing to add all these extra checks (and would welcome alternate suggestions!), but this is the only path forwards we've been able to find..




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139010