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

[Joseph Tremoulet via llvm-commits]

JosephTremoulet added a comment.

LGTM, though I'm less familiar with this part of the code.  A few nits/questions inline, but all just minor take-it-or-leave-it suggestions.


================
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)));
 }
----------------
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.

================
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);
----------------
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)

================
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();
+}
+
----------------
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.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:3344
@@ -3201,3 +3343,3 @@
       // insert a store after the PHI, so each predecessor needs to store its
-      // incoming value.
+      // incoming value
       for (unsigned i = 0, e = PN->getNumIncomingValues(); i < e; ++i) {
----------------
I don't understand this change, is there some style guideline against periods ending comments?  The other comments in the file tend to have them, and this one is even a full sentence.


http://reviews.llvm.org/D12098