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

[Thomas Lively via Phabricator via cfe-commits]

tlively added a comment.

In D66035#2178105 <https://reviews.llvm.org/D66035#2178105>, @pmatos wrote:

> Please ignore my `.gitlab-ci.yml`. That's just an internal change that I got uploaded by mistake.
> I am looking to see this through and start discussion on this with the goal of landing it.

It's great to see this patch being picked up again!

> At the moment, for example this test crashes:
>
>   struct {
>     __attribute__((address_space(256))) char *a[0];
>   } b;
>   void c() {
>     for (int d = 0; d < 10; d++)
>       b.a[d] = 0;
>   }
>
> This picked up from previous work by @vchuravy. I am still surprised by, and don't understand the reasoning behind, using a an i8 * for the externref representation. If anything I would expect to see i32 *. 
> In any case, the test above crashes in loop vectorization in `TargetLowering.h` `getValueType` because externref is casted just fine to a pointer type and it shouldn't be since `externref` is supposed to be opaque.

I'm not aware of any particular reason to use i8* and I would expect i32* to work as well. Certainly once LLVM has opaque pointers, we will want to take advantage of those. What is this test case supposed to do? As far as I can tell, it's not very meaningful to have reference types in a struct since they can't be stored in memory. Is the zero assignment supposed to become a nullref assignment?

> I would be keen to hear some comments and suggestions going forward on this.

I am curious about how you plan to use this functionality from frontends going forward and what exact use cases you are aiming to support.



================
Comment at: clang/lib/Basic/Targets/WebAssembly.cpp:43
       .Case("exception-handling", HasExceptionHandling)
+      .Case("reference-types", HasReferenceTypes)
       .Case("bulk-memory", HasBulkMemory)
----------------
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.


================
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;
----------------
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?


================
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
----------------
Do we need `funcref` here as well, or do we just use `externref` everywhere and infer separate `funcref` types later?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:4129
+    if (Offsets[i] != 0)
+      A = DAG.getNode(ISD::ADD, dl,
+                      PtrVT, Ptr,
----------------
What is this change for?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66035