[PATCH] D66035: [WebAssembly] WIP: Add support for reference types

[Paulo Matos via Phabricator via llvm-commits]

pmatos added a comment.

I will be splitting the part enabling the target feature through clang into a separate revision as suggested by @tlively



================
Comment at: clang/lib/Basic/Targets/WebAssembly.cpp:43
       .Case("exception-handling", HasExceptionHandling)
+      .Case("reference-types", HasReferenceTypes)
       .Case("bulk-memory", HasBulkMemory)
----------------
tlively wrote:
> I would split the target-features changes out into a separate patch that we can land right away. There is utility in having the feature flag available to be put into the target-features section even if LLVM cannot itself emit reference types usage yet.
Sure - I will create a separate revision for this bit then. 


================
Comment at: clang/lib/Basic/Targets/WebAssembly.cpp:183
+      HasReferenceTypes = true;
+      resetDataLayout("e-m:e-p:32:32-i64:64-n32:64-S128-ni:256");
+      continue;
----------------
tlively wrote:
> It would be good to have a comment about what is different in this data layout string. Also, is it really necessary to have a different data layout string when reference types are enabled?
ni will define 256 as a non-integral address space. This is required for externref but I don't think it hurts to have it even if reference types is disabled but it feels cleaner to not have it if not needed.


================
Comment at: llvm/include/llvm/CodeGen/ValueTypes.td:169
 def exnref: ValueType<0, 134>;      // WebAssembly's exnref type
+def externref: ValueType<0, 135>;   // WebAssembly's externref type
 def token  : ValueType<0  , 248>;   // TokenTy
----------------
tlively wrote:
> Do we need `funcref` here as well, or do we just use `externref` everywhere and infer separate `funcref` types later?
That's the plan for now. I think I haven't seen the use of having funcref here as well so I haven't added it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66035