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

[Andy Wingo via Phabricator via cfe-commits]

wingo marked 3 inline comments as done.
wingo added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1324
+        {LN->getChain(), Base}, LN->getMemoryVT(), LN->getMemOperand());
+    return DAG.getMergeValues({GlobalGet, LN->getChain()}, DL);
+  }
----------------
tlively wrote:
> 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.
I think it is 


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1324
+        {LN->getChain(), Base}, LN->getMemoryVT(), LN->getMemOperand());
+    return DAG.getMergeValues({GlobalGet, LN->getChain()}, DL);
+  }
----------------
wingo wrote:
> tlively wrote:
> > 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.
> I think it is 
Aaaah thanks for this tip!  Took some finagling to find the right formula but I got it now.


================
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 {
----------------
tlively wrote:
> 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.
Good tip; apparently it's consistent except in WebAssemblyInstrRef.td, where I had initiailly looked.  Will go ahead and fix that one too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101608