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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
