[WebAssembly] Fixed FrameBaseLocal not being set.

[aardappel]

Summary

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?

[dschuff]

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?

[aardappel]

I reduced the C++ code that generates this, not any LLVM IR, but its still a fair amount of code since it pulls in a bunch of headers. Quickest would probably to generate the IR and then construct a new test based on that, if you think its useful.

[dschuff]

It's probably worth an attempt, and that sounds like the right approach. A crash bug like this is often not too hard to reduce with bugpoint; i.e. you'd start with clang using -emit-llvm to get the IR, and then let bugpoint reduce the IR.

[aardappel]

clang-format

[aardappel]

The original .ll is 145K, which bugpoint then reduces 119K. Bugpoint is actually able to reduce most function bodies to nothing, but it leaves in all the debug information, and any attempt at reducing it either errors or makes the repro go away. Also, bugpoint makes some identifiers really long (in an attempt to encode its search space?) that would all have to be renamed. I don't see an easy way to make this an acceptable looking test case that we could add as an llvm-lit test.

[dschuff]

Hm, that's too bad. In theory it seems like it ought to be possible to just remove all the debug info from the IR that's attached to functions (i.e. all the debug.declare/debug.values) and the debug metadata that's associated with all functions other than this one, since this is a completely function-local transform that really only depends on there being any CU information at all.
I've also sometimes had good luck with c-reduce to try to reduce it at the C level rather than the IR level. It might be worth a try.
Or maybe we've let this go long enough, and we could just say that we'll have coverage if we do any continuous builds of a reasonable test suite with debug info?

[aardappel]

Even removing the debug decls at the end of the file still leaves a 37K very messy file that I would not want to add as a test :)

I am probably biased, but I don't think this is worth further effort. This is a small issue that very easily have been been written correctly initially (like so many bits of correct code for which we don't have explicit tests), it just wasn't.

I'm sure further coverage of debug functionality is yet to come.

[dschuff]

Yeah, let's just go ahead and land this. I think once this rolls we can enable DWARF with -g by default and start to get more feedback.

llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp

 char WebAssemblyExplicitLocals::ID = 0;
 INITIALIZE_PASS(WebAssemblyExplicitLocals, DEBUG_TYPE,
                 "Convert registers to WebAssembly locals", false, false)
 
 FunctionPass *llvm::createWebAssemblyExplicitLocals() {
   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,
                            WebAssemblyFunctionInfo &MFI, unsigned &CurLocal,
                            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;
 }
 
 /// Get the appropriate drop opcode for the given register class.
 static unsigned getDropOpcode(const TargetRegisterClass *RC) {
   if (RC == &WebAssembly::I32RegClass)
bool WebAssemblyExplicitLocals::runOnMachineFunction(MachineFunction &MF) {
   for (MachineBasicBlock::iterator I = MF.begin()->begin(),
                                    E = MF.begin()->end();
        I != E;) {
     MachineInstr &MI = *I++;
     if (!WebAssembly::isArgument(MI.getOpcode()))
       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;
   }
 
   // Start assigning local numbers after the last parameter.
   unsigned CurLocal = static_cast<unsigned>(MFI.getParams().size());
 
   // Precompute the set of registers that are unused, so that we can insert