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

[Thomas Lively via Phabricator via cfe-commits]

tlively 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"
----------------
pmatos wrote:
> 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.
> 
Yes, there are CPU variants defined here: https://github.com/llvm/llvm-project/blob/7ac0442fe59dbe0f9127e79e8786a7dd6345c537/llvm/lib/Target/WebAssembly/WebAssembly.td#L89-L100. Note that the CPU may enable reference types even if the feature string does not. If it doesn't break anything, then unconditionally updating the layout string sounds like the best option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104797