[PATCH] D104797: [WebAssembly] Implementation of global.get/set for reftypes in LLVM IR

[Thomas Lively via Phabricator via llvm-commits]

tlively added a comment.

Unfortunately I don't think

In D104797#2836475 <https://reviews.llvm.org/D104797#2836475>, @pmatos wrote:

> @tlively Do you think it would be ok to re-add the code removed in `ac81cb7e` but only error if the pointer is to an **opaque** non-integral type?

Unfortunately that wouldn't be future-proof given the ongoing project to remove type information from pointers. I think the best we can do right now is call `report_fatal_error` from the backend whenever a reference type is the operand or result of ptrtoint or inttoptr.



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.h:69
+    if (AS == WasmAddressSpace::EXTERNREF)
+      return MVT::externref;
+    else if (AS == WasmAddressSpace::FUNCREF)
----------------
It might be worth a comment explaining why the memtype is also externref and funcref. Is this just for lack of a more meaningful type to return?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp:130
                         TT.isArch64Bit()
-                            ? "e-m:e-p:64:64-i64:64-n32:64-S128-ni:1"
-                            : "e-m:e-p:32:32-i64:64-n32:64-S128-ni:1",
+                            ? (hasReferenceTypes(FS)
+                                   ? "e-m:e-p:64:64-i64:64-n32:64-S128-ni:1:10:20"
----------------
`hasReferenceTypes` should also be taking the CPU into account, not just the feature string. Normally this would be done via `getSubtargetImpl`, but I guess we probably can't call that this early in the construction of the `WebAssemblyTargetMachine`. Would anything break if we just unconditionally added the reference types address spaces to the data layout string?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104797