[PATCH] D75718: [WebAssembly] Fixed FrameBaseLocal not being set.

[Wouter van Oortmerssen via Phabricator via llvm-commits]

aardappel created this revision.
aardappel added a reviewer: dschuff.
Herald added subscribers: llvm-commits, sunfish, aheejin, hiraditya, sbc100.
Herald added a project: LLVM.
aardappel added a subscriber: azakai.
dschuff added a comment.

Yeah, this definitely looks like the likely fix. It's the spot where a local gets assigned that doesn't go through getLocalId. It seems to make sense to still allow this merging, as it would reduce the total number of locals. I guess it does mean that the frame base info would be incorrect when that register has the argument's value rather than the frame base's value, but presumably that would be ok, since the frame base should be live anytime a variable on the stack is live.

Were you able to reduce the IR that resulted in this to something small enough we could put in a test?


Fixes: https://bugs.llvm.org/show_bug.cgi?id=44920

WebAssemblyRegColoring may merge the vreg that currently represents
the FrameBase with one representing an argument.
WebAssemblyExplicitLocals picks up the corresponding local when
a vreg is first added to the Reg2Local mapping, except when it is
an argument instruction which are handled separately.

Note that this does not change that vregs representing the FrameBase
may get merged, it is not clear to me that this may have other
effects we may want to avoid?


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75718

Files:
  llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp


Index: llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp
===================================================================
--- llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp
+++ llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp
@@ -69,6 +69,18 @@
   return new WebAssemblyExplicitLocals();
 }
 
+static void checkFrameBase(WebAssemblyFunctionInfo &MFI, unsigned Local,
+                            unsigned Reg) {
+  // Mark a local for the frame base vreg.
+  if (MFI.isFrameBaseVirtual() && Reg == MFI.getFrameBaseVreg()) {
+    LLVM_DEBUG({
+      dbgs() << "Allocating local " << Local << "for VReg "
+             << Register::virtReg2Index(Reg) << '\n';
+    });
+    MFI.setFrameBaseLocal(Local);
+  }
+}
+
 /// Return a local id number for the given register, assigning it a new one
 /// if it doesn't yet have one.
 static unsigned getLocalId(DenseMap<unsigned, unsigned> &Reg2Local,
@@ -76,14 +88,7 @@
                            unsigned Reg) {
   auto P = Reg2Local.insert(std::make_pair(Reg, CurLocal));
   if (P.second) {
-    // Mark the local allocated for the frame base vreg.
-    if (MFI.isFrameBaseVirtual() && Reg == MFI.getFrameBaseVreg()) {
-      LLVM_DEBUG({
-        dbgs() << "Allocating local " << CurLocal << "for VReg "
-               << Register::virtReg2Index(Reg) << '\n';
-      });
-      MFI.setFrameBaseLocal(CurLocal);
-    }
+    checkFrameBase(MFI, CurLocal, Reg);
     ++CurLocal;
   }
   return P.first->second;
@@ -227,7 +232,9 @@
       break;
     Register Reg = MI.getOperand(0).getReg();
     assert(!MFI.isVRegStackified(Reg));
-    Reg2Local[Reg] = static_cast<unsigned>(MI.getOperand(1).getImm());
+    auto Local = static_cast<unsigned>(MI.getOperand(1).getImm());
+    Reg2Local[Reg] = Local;
+    checkFrameBase(MFI, Local, Reg);
     MI.eraseFromParent();
     Changed = true;
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D75718.248629.patch
Type: text/x-patch
Size: 1866 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200306/fae3b1c1/attachment.bin>