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

[Thomas Lively via Phabricator via cfe-commits]

tlively added a comment.

It would be good to split this into (at least) two patches. The first should do the minimal testable amount of work to get instruction selection working, and follow-on patches can add the other parts, like additions to the object file format. Part of the reason for that split is that different people will be better at reviewing those different parts of the patch.



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp:141-143
+  if (HasReferenceTypes(FS)) {
+    Ret += "-ni:256"; // externref
+  }
----------------
I'm not sure why we're starting with address space 256. The AMDGPU backend, for example, uses address spaces 1-7, so I think it would be ok for us to start at 1.

I also think it's somewhat strange for target features to change the datalayout. I looked at a handful of other targets and most of their data layouts were determined by the triple alone, if possible, and some considered other granular settings like the CPU. I don't really understand the discussion about compatibility that @vchuravy and @aheejin had below, though.


================
Comment at: llvm/test/CodeGen/WebAssembly/externref.ll:6
+
+declare i8 addrspace(256)* @test(i8 addrspace(256)*) 
+
----------------
This would probably be better modeled by


```
%extern = type opaque
%externref = type %extern addrspace(256)*
```

rather than using i8 pointers. The LLVM IR validator would then disallow loads and stores from `%externref`s.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66035