[PATCH] D154294: [Windows SEH]: Do not fold branches for MSVC TableSEH function

[Phoebe Wang via Phabricator via llvm-commits]

pengfei added a comment.

> I'm concerned that if we ignore the bad CFG now, it's going to come back to bite us again.

It doesn't look to me there's a bad CFG if we never do BranchFolding here. You can find the predecessors/successors chain EH BBs correctly in the CFG. The problem looks to me is BranchFolding ignores EH edges by itself.

> Can you look into adding EH edges to the relevant blocks?

Yeah, a possible solution is the fallthrough BB to inherit the successors of the BB that merging to it. But I'm worrying it affects non SEH scenarios as well. We have already used EHScopeMembership/SameEHScope to calculate for them. I think the best solution is to take SEH state number into account. But I haven't figure out the mechanism of EHScopeMembership and how to do it for SEH. So in this patch, I just skip this transform for MSVC_TableSEH.

> I'm guessing a better fix would be to block this transform in the right cases.

Thanks @rnk, so you are agreeing with this approach, right?

> BranchFolding has an explicit check for MBB.isMachineBlockAddressTaken(), though; that's supposed to prevent deleting blocks that are mentioned in side-tables.

I have tried to set AddressTaken for the BB before. The problem is the CFG has already broken by this pass, so preserve the single BB doesn't solve the real problem.

  diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  index d70de26655b9..8cf7bbfd5e9c 100644
  --- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  +++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  @@ -1802,7 +1802,9 @@ void SelectionDAGBuilder::visitCatchPad(const CatchPadInst &I) {
     bool IsCoreCLR = Pers == EHPersonality::CoreCLR;
     bool IsSEH = isAsynchronousEHPersonality(Pers);
     MachineBasicBlock *CatchPadMBB = FuncInfo.MBB;
  -  if (!IsSEH)
  +  if (IsSEH)
  +    CatchPadMBB->setMachineBlockAddressTaken();
  +  else
       CatchPadMBB->setIsEHScopeEntry();
     // In MSVC C++ and CoreCLR, catchblocks are funclets and need prologues.
     if (IsMSVCCXX || IsCoreCLR)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154294