[PATCH] D101140: [WebAssembly][CodeGen] IR support for WebAssembly local variables

[Andy Wingo via Phabricator via cfe-commits]

wingo added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/MIRYamlMapping.h:351
     IO.enumCase(ID, "scalable-vector", TargetStackID::ScalableVector);
+    IO.enumCase(ID, "object", TargetStackID::Object);
     IO.enumCase(ID, "noalloc", TargetStackID::NoAlloc);
----------------
tlively wrote:
> Is there a good test to demonstrate this change in?
Done in ir-locals-stackid.ll


================
Comment at: llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.cpp:34-36
+// runs between then and DAG building time, though, so instead we hoist stack
+// objects lazily when they are first used, and comprehensively after the DAG is
+// built via the PreprocessISelDAG hook, called by the
----------------
tlively wrote:
> Why do this in two places instead of just once comprehensively in the hook? It would be good to explain that in the comment, too.
👍 


================
Comment at: llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.cpp:63-64
+  MFI.setObjectOffset(FrameIndex, Local);
+  // Allocate WebAssembly locals for each non-aggregate component of the
+  // allocation.
+  for (EVT ValueVT : ValueVTs)
----------------
tlively wrote:
> Can there be aggregate components? What do we do with them?
Hoo, it is a good question.  I think the high-level answer is that in the same way as you can allocate an i32 to a local, or an externref to a local, you could allocate a struct { i32, externref }.  The leaves of that aggregate would be numbered from 0 to N, assigned to consecutive locals of the appropriate primitive types, and accesses would compiled down to direct access to the members.  Probably the first person who tries this is going to run into some interesting cases and I think specifically the local.get index mapping isn't quite yet ready.  But that would be the idea.  WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101140