[PATCH] D128440: [WebAssembly] Initial support for reference type funcref in clang

[Paulo Matos via Phabricator via llvm-commits]

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


================
Comment at: clang/lib/Basic/Targets/DirectX.h:45
     3, // hlsl_groupshared
+    // Wasm address space values for this map are dummy
+    20, // wasm_funcref
----------------
erichkeane wrote:
> pmatos wrote:
> > erichkeane wrote:
> > > Also apply to the rest of the targets.  Also, why is there only 1 added here?  I thought we had 2 address spaces for Wasm?
> > Externref is a type in clang, doesn't need its own address space in clang, only when it's lowered to a opaque type in llvm it gets its own address space.
> Please fix the comment in my suggestion.
> 
> Also, your explanation doesn't make it clear to me why you added TWO entries to this list for other targets, but only 1 here.
I have now fixed that. We just need one address space.


================
Comment at: clang/lib/Basic/Targets/WebAssembly.h:44-45
+    0,  // ptr64
+    10, // wasm_externref,
+    20, // wasm_funcref
+};
----------------
erichkeane wrote:
> aaron.ballman wrote:
> > I'm not super familiar with the address space maps, but this sets of my spidey senses. All of the other address space maps end with:
> > 
> > ptr64, hlsl_groupshared, wasm_funcref
> > 
> > but this one is ending with:
> > 
> > ptr64, wasm_externref, wasm_funcref
> > 
> > which makes me think something's amiss here. Thoughts?
> That DEFINITELY looks suspicious to me as well, I noted above too, but either we need to specify we're re-using the `hlsl_groupshared` for `wasm_externref` address space (at which point, i wonder: why not re-use a different address space?), or correct this.
Fixed this now. We only need the address space for funcref. The address space for externref is now necessary as it's its own type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128440