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

Thu Jul 1 12:48:51 PDT 2021

tlively accepted this revision.
tlively added a comment.
This revision is now accepted and ready to land.

Thanks!

================
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:
> > pmatos wrote:
> > > tlively wrote:
> > > > 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.
> > > Interesting - had not come accross it. Bleeding edge does not seem to include reference-types. What's the reason for this? 
> > We don't have a well-defined process for adding features to bleeding-edge, but I think typically they're added once they're mostly stable and usable in the tools.
> @tlively I have now unconditionally updated the layout string. No failures. How does this look like now?
Looks good, thanks!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104797