[PATCH] D12098: [WinEH] Calculate state numbers for the new EH representation

[Reid Kleckner via llvm-commits]

rnk added a comment.

Much excitement. :)


================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:185-188
@@ -184,6 +184,6 @@
 
-const MCExpr *WinException::create32bitRef(const GlobalValue *GV) {
-  if (!GV)
+const MCExpr *WinException::create32bitRef(const Value *V) {
+  if (!V)
     return MCConstantExpr::create(0, Asm->OutContext);
-  return create32bitRef(Asm->getSymbol(GV));
+  return create32bitRef(Asm->getSymbol(cast<GlobalValue>(V)));
 }
----------------
JosephTremoulet wrote:
> Nit: I don't understand this change; the function will fail if you pass it a non-null non-GlobalValue Value, so to me it seems less error prone to type the parameter as GlobalValue and force callers to recognize the need to downcast.
Right, the intent is for the assertion to fail at this point once we start passing such values through. Previously, we outlined funclets in IR, and the IR function was the GlobalValue that we passed through. With the new representation, we are passing a BasicBlock, which inherits from Value. We can't actually reach this code today because SDAGBuilder can't select our new pad instructions yet. Eventually we will get here, the assertion will fail, and we'll fix it up and give it the correct BasicBlock type.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:2581
@@ -2580,6 +2580,3 @@
   UME.ToState = ToState;
-  if (auto *CH = dyn_cast_or_null<CleanupHandler>(AH))
-    UME.Cleanup = cast<Function>(CH->getHandlerBlockOrFunc());
-  else
-    UME.Cleanup = nullptr;
+  UME.Cleanup = const_cast<Value *>(V);
   FuncInfo.UnwindMap.push_back(UME);
----------------
JosephTremoulet wrote:
> Nit: const_cast is unfortunate.  Would it be horrible to push constness down or non-constness up?  (I'm not familiar with the callers/callees imposing the restrictions here)
Pushing it down is the right thing in this case. The unwind map lives through codegen, and there shouldn't be non-const IR pointers hanging around during codegen.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:2949-2958
@@ +2948,12 @@
+
+static const BasicBlock *getUnwindPredecessor(const BasicBlock *BB) {
+  const TerminatorInst *TI = BB->getTerminator();
+  if (isa<InvokeInst>(TI))
+    return nullptr;
+  if (isa<CatchPadInst>(TI) || isa<CatchEndPadInst>(TI) ||
+      isa<TerminatePadInst>(TI))
+    return BB;
+  return cast<CleanupPadInst>(cast<CleanupReturnInst>(TI)->getReturnValue())
+      ->getParent();
+}
+
----------------
JosephTremoulet wrote:
> Nit: this was a confusing read because it's not getting the unwind pred of BB, it's getting the unwind pred of a successor of BB.  I think it would be easier to read if the parameter name were PredBB and/or a comment explained.
I guess it really does the following:
  Given BB which ends in an unwind edge, return the EHPad that this BB belongs to. If the unwind edge came from an invoke, return null.

Call it getEHPadFromPredecessor?

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:3284
@@ +3283,3 @@
+
+  WinEHFuncInfo FuncInfo;
+
----------------
Also left over from hurried pair programming. :)

================
Comment at: lib/Target/X86/X86WinEHState.cpp:400-405
@@ +399,8 @@
+void WinEHStatePass::addCXXStateStores(Function &F, MachineModuleInfo *MMI) {
+  WinEHFuncInfo *FuncInfoPtr;
+  WinEHFuncInfo Storage;
+  if (MMI)
+    FuncInfoPtr = &MMI->getWinEHFuncInfo(&F);
+  else
+    FuncInfoPtr = &Storage;
+  WinEHFuncInfo &FuncInfo = *FuncInfoPtr;
----------------
Probably worth commenting that we only ever create our own WinEHFuncInfo for testing purposes. MMI is non-null iff we are running as part of a CodeGen pass manager.

Given that it's for testing only... we should heap allocate it with something like this:
  // If MMI is null, create our own WinEHFuncInfo. This only happens in opt tests.
  std::unique_ptr<WinEHFuncInfo> FuncInfoPtr;
  if (!MMI)
    FuncInfoPtr.reset(new WinEHFuncInfo());
  WinEHFuncInfo &FuncInfo = *(MMI ? &MMI->getWinEHFuncInfo(&F) : FuncInfoPtr.get());

================
Comment at: lib/Target/X86/X86WinEHState.cpp:496-501
@@ +495,8 @@
+void WinEHStatePass::addSEHStateStores(Function &F, MachineModuleInfo *MMI) {
+  WinEHFuncInfo *FuncInfoPtr;
+  WinEHFuncInfo Storage;
+  if (MMI)
+    FuncInfoPtr = &MMI->getWinEHFuncInfo(&F);
+  else
+    FuncInfoPtr = &Storage;
+  WinEHFuncInfo &FuncInfo = *FuncInfoPtr;
----------------
Both of these functions only consume WinEHFuncInfo. We should hoist this logic up to runOnFunction and only pass down WinEHFuncInfo.


http://reviews.llvm.org/D12098