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

Mon Jun 28 12:29:53 PDT 2021

pmatos added inline comments.

================
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"
----------------
tlively wrote:
> `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?
Regarding this change, I don't quite understand why referencetypes should take the CPU into account. Are there CPU variants for the wasm backend? I haven't touched the conditional because that would mean touching the several tests that don't enable reference types and use the old data layout string. However, I would think that if that's the path we want to follow here, we could do it and change all wasm tests to use the layout string with reference types.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104797