[WebAssembly] Track frame registers through VReg and local allocation

[dschuff]

Summary

This change has 2 components:

Target-independent:
add a method getDwarfFrameBase to TargetFrameLowering. It describes how the Dwarf frame base will be encoded.
That can be a register (the default), the CFA (which replaces NVPTX-specific logic in DwarfCompileUnit), or a DW_OP_WASM_location descriptr.

WebAssembly:
Allow WebAssemblyFunctionInfo::getFrameRegister to return the correct virtual register instead of FP32/SP32 after WebAssemblyReplacePhysRegs has run.
Make WebAssemblyExplicitLocals store the local it allocates for the frame register. Use this local information to implement getDwarfFrameBase

The result is that the DW_AT_frame_base attribute is correctly encoded for each subprogram, and each param and local variable has a correct DW_AT_location that uses DW_OP_fbreg to refer to the frame base.

[dschuff]

I think that this approach to tracking the frame register through VReg and local substitution will work fairly reliably for now, because the frame register is live through the whole function and is unlikely to be coalesced with other locals or stackified.
One thing I don't like about D69807 is that getFrameRegister is called in a variety of places during generation of various bits of the code as well as in DwarfCompileUnit, whereas getFrameBaseLocation only has this particular meaning and used here (while in the other places getFrameRegister is returning phsyical registers that are meaningless partway through the pipeline.

I had hoped that I could avoid introducing a cross-cutting abstraction like getFrameBaseLocation by having the DWARF code either check the target (as it does with NVPTX) or check whether getFrameRegister returns a physreg (as it does currently, in order to ignore vregs); but then we still don't have a way to actually get the local number to encode, because it's stored in a data structure that's in the WebAssembly backend and may not even be compiled in.
So I'm not sure if we can find a good way to get it from DwarfCompileUnit.

[dschuff]

• use getDwarfFrameBase on WebAssemblyFrameLowering
• Don't optimize live intervale for frame reg. use global instead of local when not needsSP
• Update NVPTX

[dschuff]

• update tests, add new test for dwarf local var
• start update multi-return test

[dschuff]

llvm/test/CodeGen/WebAssembly/multi-return.ll

1 ↗

(On Diff #236726)

@tlively I re-ran update_llc_test_checks.py over this file and it removed all the CHECKs entirely (best kind of correct...). Am I holding it wrong?

15 ↗

(On Diff #236726)

The reason all the tests in this file change is because I exempted the frame registers from OptimizeLiveIntervals.
An alternative to just updating it would be to do what I did for the first 2 tests and use symbolic variables for the registers. @tlively are these reasonable names? Are these exactly the correct CHECKs? (It's slightly non-obvious exactly what these are checking)

[tlively]

Unfortunately I do not know how multi-return.ll works. Despite its name, it is not a multivalue test (although once multivalue works it would make sense to add a multivalue-enabled run to that file). I generally don't use test-generation scripts because they tend to produce tests with unclear goals, as you observe.

[dschuff]

• finish update multi-return
• remove debug prints
• clean up, add comments
• clang-format

[dschuff]

• add doxygen comment

[dschuff]

This is ready for review now, please take a look.

Note for the wasm-backend folks:
The original virtual register philosophy in the backend was to eliminate the physical registers early and treat all the registers the same, which made things simpler. This CL walks back on that strategy a little bit, treating the frame registers specially in a couple of places. Currently the changes are quite simple but it's possible that we may find more places where we have to take those registers into account to get good debug information. An alternative might be to go back to having physical registers, which I'm guessing would add some complexity in a few places (which would be different places than the ones this CL introduces); it's not obvious which would be better.

I think in the longer term though that we might just want to use Dwarf CFI instead, and use the CFA instead of an explicit register as the frame base.
Even if we do that though, I think the TargetFrameLowering change is a good one because it eliminates the NVPTX oddity (and if we go to CFI we'd be using the same code). And this should cover the large majority of debugging cases in the short term, so it's worth putting in for now.

[dschuff]

also cc @paolosev and @philip.pfaffe

[dschuff]

Rebase

• suppress build warning

[dschuff]

• Fix NVPTX

[dschuff]

Ping.

[yurydelendik]

Agree with all the changes

[dschuff]

Thanks, I'll commit this soon.

[dschuff]

• Cleanup scope, update lld debuginfo test

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

DIE &DwarfCompileUnit::updateSubprogramScopeDIE(const DISubprogram *SP) {
   if (!includeMinimalInlineScopes()) {
     if (Asm->MF->getTarget().getTargetTriple().isNVPTX()) {
       DIELoc *Loc = new (DIEValueAllocator) DIELoc;
       addUInt(*Loc, dwarf::DW_FORM_data1, dwarf::DW_OP_call_frame_cfa);
       addBlock(*SPDie, dwarf::DW_AT_frame_base, Loc);
     } else {
       const TargetRegisterInfo *RI = Asm->MF->getSubtarget().getRegisterInfo();
       MachineLocation Location(RI->getFrameRegister(*Asm->MF));
-      if (Register::isPhysicalRegister(Location.getReg()))
+      if (Register::isPhysicalRegister(Location.getReg())) {
         addAddress(*SPDie, dwarf::DW_AT_frame_base, Location);
+      } else {
+        WebAssemblyFunctionInfo &MFI = *MF.getInfo<WebAssemblyFunctionInfo>(); // oops, layering violation
+        // I'd like to encode a dwarf op_wasm_location here to encode the local value using something like
+        // MFI->getFrameBaseLocal() but that's in the wasm backend and not target-independent.
+      }
     }
   }
 
   // Add name to the name table, we do this here because we're guaranteed
   // to have concrete versions of our DW_TAG_subprogram nodes.
   DD->addSubprogramNames(*CUNode, SP, *SPDie);
 
   return *SPDie;

llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp

 
 FunctionPass *llvm::createWebAssemblyExplicitLocals() {
   return new WebAssemblyExplicitLocals();
 }
 
 /// 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)
+  if (P.second) {
+    if (Reg == MFI.getFrameBaseVreg())
+      MFI.setFrameBaseLocal(CurLocal);
     ++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)
     return WebAssembly::DROP_I32;
   if (RC == &WebAssembly::I64RegClass)
for (MachineBasicBlock::iterator I = MBB.begin(), E = MBB.end(); I != E;) {
       if (WebAssembly::isTee(MI.getOpcode())) {
         assert(MFI.isVRegStackified(MI.getOperand(0).getReg()));
         assert(!MFI.isVRegStackified(MI.getOperand(1).getReg()));
         Register OldReg = MI.getOperand(2).getReg();
         const TargetRegisterClass *RC = MRI.getRegClass(OldReg);
 
         // Stackify the input if it isn't stackified yet.
         if (!MFI.isVRegStackified(OldReg)) {
-          unsigned LocalId = getLocalId(Reg2Local, CurLocal, OldReg);
+          unsigned LocalId = getLocalId(Reg2Local, MFI, CurLocal, OldReg);
           Register NewReg = MRI.createVirtualRegister(RC);
           unsigned Opc = getLocalGetOpcode(RC);
           BuildMI(MBB, &MI, MI.getDebugLoc(), TII->get(Opc), NewReg)
               .addImm(LocalId);
           MI.getOperand(2).setReg(NewReg);
           MFI.stackifyVReg(NewReg);
         }
 
         // Replace the TEE with a LOCAL_TEE.
         unsigned LocalId =
-            getLocalId(Reg2Local, CurLocal, MI.getOperand(1).getReg());
+            getLocalId(Reg2Local, MFI, CurLocal, MI.getOperand(1).getReg());
         unsigned Opc = getLocalTeeOpcode(RC);
         BuildMI(MBB, &MI, MI.getDebugLoc(), TII->get(Opc),
                 MI.getOperand(0).getReg())
             .addImm(LocalId)
             .addReg(MI.getOperand(2).getReg());
 
         WebAssemblyDebugValueManager(&MI).replaceWithLocal(LocalId);
 
for (MachineBasicBlock::iterator I = MBB.begin(), E = MBB.end(); I != E;) {
           if (UseEmpty[Register::virtReg2Index(OldReg)]) {
             unsigned Opc = getDropOpcode(RC);
             MachineInstr *Drop =
                 BuildMI(MBB, InsertPt, MI.getDebugLoc(), TII->get(Opc))
                     .addReg(NewReg);
             // After the drop instruction, this reg operand will not be used
             Drop->getOperand(0).setIsKill();
           } else {
-            unsigned LocalId = getLocalId(Reg2Local, CurLocal, OldReg);
+            unsigned LocalId = getLocalId(Reg2Local, MFI, CurLocal, OldReg);
             unsigned Opc = getLocalSetOpcode(RC);
 
             WebAssemblyDebugValueManager(&MI).replaceWithLocal(LocalId);
 
             BuildMI(MBB, InsertPt, MI.getDebugLoc(), TII->get(Opc))
                 .addImm(LocalId)
                 .addReg(NewReg);
           }
for (MachineBasicBlock::iterator I = MBB.begin(), E = MBB.end(); I != E;) {
 
         Register OldReg = MO.getReg();
 
         // Inline asm may have a def in the middle of the operands. Our contract
         // with inline asm register operands is to provide local indices as
         // immediates.
         if (MO.isDef()) {
           assert(MI.isInlineAsm());
-          unsigned LocalId = getLocalId(Reg2Local, CurLocal, OldReg);
+          unsigned LocalId = getLocalId(Reg2Local, MFI, CurLocal, OldReg);
           // If this register operand is tied to another operand, we can't
           // change it to an immediate. Untie it first.
           MI.untieRegOperand(MI.getOperandNo(&MO));
           MO.ChangeToImmediate(LocalId);
           continue;
         }
 
         // If we see a stackified register, prepare to insert subsequent
         // local.gets before the start of its tree.
         if (MFI.isVRegStackified(OldReg)) {
           InsertPt = findStartOfTree(MO, MRI, MFI);
           continue;
         }
 
         // Our contract with inline asm register operands is to provide local
         // indices as immediates.
         if (MI.isInlineAsm()) {
-          unsigned LocalId = getLocalId(Reg2Local, CurLocal, OldReg);
+          unsigned LocalId = getLocalId(Reg2Local, MFI, CurLocal, OldReg);
           // Untie it first if this reg operand is tied to another operand.
           MI.untieRegOperand(MI.getOperandNo(&MO));
           MO.ChangeToImmediate(LocalId);
           continue;
         }
 
         // Insert a local.get.
-        unsigned LocalId = getLocalId(Reg2Local, CurLocal, OldReg);
+        unsigned LocalId = getLocalId(Reg2Local, MFI, CurLocal, OldReg);
         const TargetRegisterClass *RC = MRI.getRegClass(OldReg);
         Register NewReg = MRI.createVirtualRegister(RC);
         unsigned Opc = getLocalGetOpcode(RC);
         InsertPt =
             BuildMI(MBB, InsertPt, MI.getDebugLoc(), TII->get(Opc), NewReg)
                 .addImm(LocalId);
         MO.setReg(NewReg);
         MFI.stackifyVReg(NewReg);

llvm/lib/Target/WebAssembly/WebAssemblyMachineFunctionInfo.h

class WebAssemblyFunctionInfo final : public MachineFunctionInfo {
   // A virtual register holding the pointer to the vararg buffer for vararg
   // functions. It is created and set in TLI::LowerFormalArguments and read by
   // TLI::LowerVASTART
   unsigned VarargVreg = -1U;
 
   // A virtual register holding the base pointer for functions that have
   // overaligned values on the user stack.
   unsigned BasePtrVreg = -1U;
+  // A virtual register holding the frame base. This is either FP or SP
+  // after it has been replaced by a vreg
+  unsigned FrameBaseVreg = -1U;
+  // The local holding the frame base. This is either FP or SP
+  // after WebAssemblyExplicitLocals
+  unsigned FrameBaseLocal = -1U;
 
   // Function properties.
   bool CFGStackified = false;
 
 public:
   explicit WebAssemblyFunctionInfo(MachineFunction &MF) : MF(MF) {}
   ~WebAssemblyFunctionInfo() override;
   void initializeBaseYamlFields(const yaml::WebAssemblyFunctionInfo &YamlMFI);
unsigned getVarargBufferVreg() const {
     return VarargVreg;
   }
   void setVarargBufferVreg(unsigned Reg) { VarargVreg = Reg; }
 
   unsigned getBasePointerVreg() const {
     assert(BasePtrVreg != -1U && "Base ptr vreg hasn't been set");
     return BasePtrVreg;
   }
+  void setFrameBaseVreg(unsigned Reg) { FrameBaseVreg = Reg; }
+  unsigned getFrameBaseVreg() const {
+    //assert(FrameBaseVreg != -1U && "Frame base vreg hasn't been set");
+    return FrameBaseVreg;
+  }
+  void setFrameBaseLocal(unsigned Local) { FrameBaseLocal = Local; }
+  unsigned getFrameBaseLocal() const {
+    assert(FrameBaseLocal != -1U && "Frame base local hasn't been set");
+    return FrameBaseLocal;
+  }
   void setBasePointerVreg(unsigned Reg) { BasePtrVreg = Reg; }
 
   static const unsigned UnusedReg = -1u;
 
   void stackifyVReg(unsigned VReg) {
     assert(MF.getRegInfo().getUniqueVRegDef(VReg));
     auto I = Register::virtReg2Index(VReg);
     if (I >= VRegStackified.size())

llvm/lib/Target/WebAssembly/WebAssemblyRegisterInfo.h

public:
   // Debug information queries.
   Register getFrameRegister(const MachineFunction &MF) const override;
 
   const TargetRegisterClass *
   getPointerRegClass(const MachineFunction &MF,
                      unsigned Kind = 0) const override;
   // This does not apply to wasm.
   const uint32_t *getNoPreservedMask() const override { return nullptr; }
+
 };
 
 } // end namespace llvm
 
 #endif

llvm/lib/Target/WebAssembly/WebAssemblyRegisterInfo.cpp

BuildMI(MBB, *II, II->getDebugLoc(), TII->get(WebAssembly::ADD_I32),
         .addReg(FrameRegister)
         .addReg(OffsetOp);
   }
   MI.getOperand(FIOperandNum).ChangeToRegister(FIRegOperand, /*isDef=*/false);
 }
 
 Register
 WebAssemblyRegisterInfo::getFrameRegister(const MachineFunction &MF) const {
+    // If the PReg has been replaced by a VReg, return that.
+  auto FI = MF.getInfo<WebAssemblyFunctionInfo>();
+  if (FI->getFrameBaseVreg() != -1) // TODO is there a better check for this?
+    return FI->getFrameBaseVreg();
   static const unsigned Regs[2][2] = {
       /*            !isArch64Bit       isArch64Bit      */
       /* !hasFP */ {WebAssembly::SP32, WebAssembly::SP64},
       /*  hasFP */ {WebAssembly::FP32, WebAssembly::FP64}};
   const WebAssemblyFrameLowering *TFI = getFrameLowering(MF);
   return Regs[TFI->hasFP(MF)][TT.isArch64Bit()];
 }
 
 const TargetRegisterClass *
 WebAssemblyRegisterInfo::getPointerRegClass(const MachineFunction &MF,
                                             unsigned Kind) const {
   assert(Kind == 0 && "Only one kind of pointer on WebAssembly");
   if (MF.getSubtarget<WebAssemblySubtarget>().hasAddr64())
     return &WebAssembly::I64RegClass;
   return &WebAssembly::I32RegClass;
 }

llvm/lib/Target/WebAssembly/WebAssemblyReplacePhysRegs.cpp

 
 bool WebAssemblyReplacePhysRegs::runOnMachineFunction(MachineFunction &MF) {
   LLVM_DEBUG({
     dbgs() << "********** Replace Physical Registers **********\n"
            << "********** Function: " << MF.getName() << '\n';
   });
 
   MachineRegisterInfo &MRI = MF.getRegInfo();
-  const auto &TRI = *MF.getSubtarget<WebAssemblySubtarget>().getRegisterInfo();
+  auto &TRI = *MF.getSubtarget<WebAssemblySubtarget>().getRegisterInfo();
   bool Changed = false;
 
   assert(!mustPreserveAnalysisID(LiveIntervalsID) &&
          "LiveIntervals shouldn't be active yet!");
   // We don't preserve SSA or liveness.
   MRI.leaveSSA();
   MRI.invalidateLiveness();
 
   for (unsigned PReg = WebAssembly::NoRegister + 1;
        PReg < WebAssembly::NUM_TARGET_REGS; ++PReg) {
     // Skip fake registers that are never used explicitly.
     if (PReg == WebAssembly::VALUE_STACK || PReg == WebAssembly::ARGUMENTS)
       continue;
 
     // Replace explicit uses of the physical register with a virtual register.
     const TargetRegisterClass *RC = TRI.getMinimalPhysRegClass(PReg);
     unsigned VReg = WebAssembly::NoRegister;
     for (auto I = MRI.reg_begin(PReg), E = MRI.reg_end(); I != E;) {
       MachineOperand &MO = *I++;
       if (!MO.isImplicit()) {
-        if (VReg == WebAssembly::NoRegister)
+        if (VReg == WebAssembly::NoRegister) {
           VReg = MRI.createVirtualRegister(RC);
+          if (PReg == TRI.getFrameRegister(MF)) {
+            auto FI = MF.getInfo<WebAssemblyFunctionInfo>();
+            assert(FI->getFrameBaseVreg() == -1U);
+            FI->setFrameBaseVreg(VReg);
+            errs() << "replacing reg " << PReg << " with " << VReg<< " (" << Register::virtReg2Index(VReg)<<
+                ")\n";
+          }
+        }
         MO.setReg(VReg);
         if (MO.getParent()->isDebugValue())
           MO.setIsDebug();
         Changed = true;
       }
     }
   }
 
   return Changed;
 }