[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

[Thomas Lively via Phabricator via cfe-commits]

tlively added a comment.

Nice! I think that splitting this out of the change that adds reference types is a good idea. Regarding the FIXME in the test, is it the case that the globals are also not emitted in the binary format, or is it just the assembly output that is broken?



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1324
+        {LN->getChain(), Base}, LN->getMemoryVT(), LN->getMemOperand());
+    return DAG.getMergeValues({GlobalGet, LN->getChain()}, DL);
+  }
----------------
I'm not sure it's necessary to create a MERGE_NODES node here, since the the GlobalGet already contains the chain. For a similar example, see how LOAD_SPLAT nodes are created and returned.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td:268
 // are implied by virtual register uses and defs.
-multiclass LOCAL<WebAssemblyRegClass vt, Operand global_op> {
+multiclass LOCAL<WebAssemblyRegClass reg, Operand global_op> {
   let hasSideEffects = 0 in {
----------------
In other places we have used the abbreviation `rc` as the name for WebAssemblyRegClass parameters, but I don't know if we do that everywhere.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td:328
+  } // hasSideEffects = 0
+  foreach vt = reg.RegTypes in {
+    def : Pat<(vt (WebAssemblyglobal_get
----------------
Ahhhh, I did not know you could do this! Very cool.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101608