[WebAssembly] Delete ThrowUnwindDest map from WasmEHFuncInfo

[aheejin]

Summary

Before when we implemented the first EH proposal, 'catch <tag>'
instruction may not catch an exception so there were multiple EH pads an
exception can unwind to. That means a BB could have multiple EH pad
successors.

Now after we switched to the new proposal, every 'catch' instruction
catches an exception, and there is only one catchpad per catchswitch, so
we at most have one EH pad successor, making ThrowUnwindDest map in
WasmEHInfo unnecessary.

Keeping ThrowUnwindDest map in WasmEHInfo has its own problems,
because other optimization passes can split a BB that contains possibly
throwing calls (previously invokes), and we have to update the map every
time that happens, which is not easy for common CodeGen passes.

This also correctly updates successor info in LateEHPrepare when we add
a rethrow instruction.

[aheejin]

• Add successor relationship

[aheejin]

It's hard to add a test for this because WasmEHFuncInfo is not serialized (All machine-specific function info is not serialized in mir or anywhere at the moment). But I wanted to split this from the other CL that uses this part because 1. that CL is already big 2. this is a bugfix

[dschuff]

You mention that this is a bugfix. what's the bug that is fixed? An optimization not keeping the map up to date?

[aheejin]

Kind of. I found it was not updated properly in LateEHPrepare so I created D58474, and abandoned it because it's better not to use ThrowUnwindDestMap anyway, because that kind of BB splitting can occur in other non-wasm passes as well, and now we can use successor info instead because there is at most one EH pad successor for a BB. (This was not the case for the first proposal.)
But we cannot remove EHPadUnwindDest because it is used here to set the new successor for newly added rethrow instruction. And this info is I think safe from transformation from other passes because both of them are EH pads, and all passes I have seen refrain from doing optimization on EH pads. But maybe there will be some passes that does that.. I can't be 100% sure.

[aheejin]

Oh and failure to update the newly created BB successor relationship by rethrow is a bug in itself, regardless of whether we use ThrowUnwindDestMap or not.

[dschuff]

Makes sense.
Is EHPadUnwindMap analogous to any of the similarly-named data structures for WinEH? If so, that would seem to make it even less likely to accidentally break, since any change that breaks it would also break WinEH.

[aheejin]

Oh turned out this (of course) changes an existing test because successor relationship was modified, which was also a pre-existing bug in the test. :( Will update the test soon.

[aheejin]

Update test

[dschuff]

still LGTM

[aheejin]

In D58486#1407525, @dschuff wrote:

Makes sense.
Is EHPadUnwindMap analogous to any of the similarly-named data structures for WinEH? If so, that would seem to make it even less likely to accidentally break, since any change that breaks it would also break WinEH.

Not exactly. (I tried but) I don't understand their structure 100% but it looks like they assign state numbers to layers of BB groups created by EH pads. So for example an EH pad has a state number 1, within BBs that's dominated by that EH pad, the next level EH pads are assigned number 2, and so on. But anyway, I think as long as this nested structure of EH pads are preserved we are ok.

(And I thought I had to change something but I didn't, so I'm changing the status back)

include/llvm/CodeGen/WasmEHFuncInfo.h

 enum EventTag { CPP_EXCEPTION = 0, C_LONGJMP = 1 };
 
 using BBOrMBB = PointerUnion<const BasicBlock *, MachineBasicBlock *>;
 
 struct WasmEHFuncInfo {
   // When there is an entry <A, B>, if an exception is not caught by A, it
   // should next unwind to the EH pad B.
   DenseMap<BBOrMBB, BBOrMBB> EHPadUnwindMap;
-  // For entry <A, B>, A is a BB with an instruction that may throw
-  // (invoke/cleanupret in LLVM IR, call/rethrow in the backend) and B is an EH
-  // pad that A unwinds to.
-  DenseMap<BBOrMBB, BBOrMBB> ThrowUnwindMap;
 
   // Helper functions
   const BasicBlock *getEHPadUnwindDest(const BasicBlock *BB) const {
     return EHPadUnwindMap.lookup(BB).get<const BasicBlock *>();
   }
   void setEHPadUnwindDest(const BasicBlock *BB, const BasicBlock *Dest) {
     EHPadUnwindMap[BB] = Dest;
   }
-  const BasicBlock *getThrowUnwindDest(BasicBlock *BB) const {
-    return ThrowUnwindMap.lookup(BB).get<const BasicBlock *>();
-  }
-  void setThrowUnwindDest(const BasicBlock *BB, const BasicBlock *Dest) {
-    ThrowUnwindMap[BB] = Dest;
-  }
   bool hasEHPadUnwindDest(const BasicBlock *BB) const {
     return EHPadUnwindMap.count(BB);
   }
-  bool hasThrowUnwindDest(const BasicBlock *BB) const {
-    return ThrowUnwindMap.count(BB);
-  }
 
   MachineBasicBlock *getEHPadUnwindDest(MachineBasicBlock *MBB) const {
     return EHPadUnwindMap.lookup(MBB).get<MachineBasicBlock *>();
   }
   void setEHPadUnwindDest(MachineBasicBlock *MBB, MachineBasicBlock *Dest) {
     EHPadUnwindMap[MBB] = Dest;
   }
-  MachineBasicBlock *getThrowUnwindDest(MachineBasicBlock *MBB) const {
-    return ThrowUnwindMap.lookup(MBB).get<MachineBasicBlock *>();
-  }
-  void setThrowUnwindDest(MachineBasicBlock *MBB, MachineBasicBlock *Dest) {
-    ThrowUnwindMap[MBB] = Dest;
-  }
   bool hasEHPadUnwindDest(MachineBasicBlock *MBB) const {
     return EHPadUnwindMap.count(MBB);
   }
-  bool hasThrowUnwindDest(MachineBasicBlock *MBB) const {
-    return ThrowUnwindMap.count(MBB);
-  }
 };
 
 // Analyze the IR in the given function to build WasmEHFuncInfo.
 void calculateWasmEHInfo(const Function *F, WasmEHFuncInfo &EHInfo);
 
 } // namespace llvm
 
 #endif // LLVM_CODEGEN_WASMEHFUNCINFO_H

lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp

else if (Personality == EHPersonality::Wasm_CXX) {
     // Map all BB references in the WinEH data to MBBs.
     DenseMap<BBOrMBB, BBOrMBB> NewMap;
     for (auto &KV : EHInfo.EHPadUnwindMap) {
       const auto *Src = KV.first.get<const BasicBlock *>();
       const auto *Dst = KV.second.get<const BasicBlock *>();
       NewMap[MBBMap[Src]] = MBBMap[Dst];
     }
     EHInfo.EHPadUnwindMap = std::move(NewMap);
-    NewMap.clear();
-    for (auto &KV : EHInfo.ThrowUnwindMap) {
-      const auto *Src = KV.first.get<const BasicBlock *>();
-      const auto *Dst = KV.second.get<const BasicBlock *>();
-      NewMap[MBBMap[Src]] = MBBMap[Dst];
-    }
-    EHInfo.ThrowUnwindMap = std::move(NewMap);
   }
 }
 
 /// clear - Clear out all the function-specific state. This returns this
 /// FunctionLoweringInfo to an empty state, ready to be used for a
 /// different function.
 void FunctionLoweringInfo::clear() {
   MBBMap.clear();

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

EHPersonality Personality =
     classifyEHPersonality(FuncInfo.Fn->getPersonalityFn());
   bool IsMSVCCXX = Personality == EHPersonality::MSVC_CXX;
   bool IsCoreCLR = Personality == EHPersonality::CoreCLR;
   bool IsWasmCXX = Personality == EHPersonality::Wasm_CXX;
   bool IsSEH = isAsynchronousEHPersonality(Personality);
 
   if (IsWasmCXX) {
     findWasmUnwindDestinations(FuncInfo, EHPadBB, Prob, UnwindDests);
+    assert(UnwindDests.size() <= 1 &&
+           "There should be at most one unwind destination for wasm");
     return;
   }
 
   while (EHPadBB) {
     const Instruction *Pad = EHPadBB->getFirstNonPHI();
     BasicBlock *NewEHPadBB = nullptr;
     if (isa<LandingPadInst>(Pad)) {
       // Stop on landingpads. They are not funclets.

lib/CodeGen/WasmEHPrepare.cpp

if (const auto *CatchPad = dyn_cast<CatchPadInst>(Pad)) {
       const Instruction *UnwindPad = UnwindBB->getFirstNonPHI();
       if (const auto *CatchSwitch = dyn_cast<CatchSwitchInst>(UnwindPad))
         // Currently there should be only one handler per a catchswitch.
         EHInfo.setEHPadUnwindDest(&BB, *CatchSwitch->handlers().begin());
       else // cleanuppad
         EHInfo.setEHPadUnwindDest(&BB, UnwindBB);
     }
   }
-
-  // Record the unwind destination for invoke and cleanupret instructions.
-  for (const auto &BB : *F) {
-    const Instruction *TI = BB.getTerminator();
-    BasicBlock *UnwindBB = nullptr;
-    if (const auto *Invoke = dyn_cast<InvokeInst>(TI))
-      UnwindBB = Invoke->getUnwindDest();
-    else if (const auto *CleanupRet = dyn_cast<CleanupReturnInst>(TI))
-      UnwindBB = CleanupRet->getUnwindDest();
-    if (!UnwindBB)
-      continue;
-    const Instruction *UnwindPad = UnwindBB->getFirstNonPHI();
-    if (const auto *CatchSwitch = dyn_cast<CatchSwitchInst>(UnwindPad))
-      // Currently there should be only one handler per a catchswitch.
-      EHInfo.setThrowUnwindDest(&BB, *CatchSwitch->handlers().begin());
-    else // cleanuppad
-      EHInfo.setThrowUnwindDest(&BB, UnwindBB);
-  }
 }

lib/Target/WebAssembly/WebAssemblyLateEHPrepare.cpp

if (TerminatePads.count(EHPad)) {
       BuildMI(ElseMBB, DL, TII.get(WebAssembly::CALL_VOID))
           .addGlobalAddress(ClangCallTerminateFn)
           .addImm(0);
       BuildMI(ElseMBB, DL, TII.get(WebAssembly::UNREACHABLE));
 
     } else {
       BuildMI(ElseMBB, DL, TII.get(WebAssembly::RETHROW));
       if (EHInfo->hasEHPadUnwindDest(EHPad))
-        EHInfo->setThrowUnwindDest(ElseMBB, EHInfo->getEHPadUnwindDest(EHPad));
+        ElseMBB->addSuccessor(EHInfo->getEHPadUnwindDest(EHPad));
     }
   }
 
   return true;
 }
 
 // After the stack is unwound due to a thrown exception, the __stack_pointer
 // global can point to an invalid address. This inserts instructions that

test/CodeGen/WebAssembly/cfg-stackify-eh.ll

 ; CHECK:         br        2                           # 2: down to label6
 ; CHECK:       catch
 ; CHECK:         try
 ; CHECK:           block
 ; CHECK:             br_if     0, {{.*}}               # 0: down to label11
 ; CHECK:             i32.call  $drop=, __cxa_begin_catch
 ; CHECK:             try
 ; CHECK:               call      foo
-; CHECK:               br        2                     # 2: down to label10
+; CHECK:               br        2                     # 2: down to label9
 ; CHECK:             catch
 ; CHECK:               call      __cxa_end_catch
 ; CHECK:               rethrow                         # down to catch3
 ; CHECK:             end_try
 ; CHECK:           end_block                           # label11:
 ; CHECK:           call      __cxa_rethrow
 ; CHECK:           unreachable
 ; CHECK:         catch     {{.*}}                      # catch3:
 ; CHECK:           call      __cxa_end_catch
 ; CHECK:           rethrow                             # to caller
-; CHECK:         end_try                               # label10:
+; CHECK:         end_try                               # label9:
 ; CHECK:         call      __cxa_end_catch
 ; CHECK:         br        2                           # 2: down to label6
 ; CHECK:       end_try
 ; CHECK:     end_block                                 # label7:
 ; CHECK:     call      __cxa_rethrow
 ; CHECK:     unreachable
 ; CHECK:   end_block                                   # label6:
 ; CHECK:   call      __cxa_end_catch