[llvm-commits] [PATCH] Newest LandingPad Patch

[Chris Lattner]

On Aug 7, 2011, at 7:38 PM, Bill Wendling wrote:

> Ping?

Sorry for the delay.  It looks like you haven't updated the patch to match LangRef to know that clauses can't have multiple values.  This makes it very hard to usefully review the patch, but here are some thoughts that are independent of that:


+++ include/llvm/Instructions.h	(working copy)

+class LandingPadInst : public Instruction {

+  /// IsCleanup - True if the landingpad instruction is also a cleanup.
+  bool IsCleanup;

This should be stored in "SubClassData" like the isVolatile bit on loads and stores are.


Please move the Create functions and constructors out of line.

LandingPadInst is overcomplicated (e.g. struct Index) by the grammar that you're implementing that isn't documented.


@@ -2617,6 +2736,10 @@
     Op<-1>() = reinterpret_cast<Value*>(B);
   }
 
+  // getLandingPad - Get the landingpad instruction from the landing pad block
+  // (the unwind destination).
+  LandingPadInst *getLandingPad() const;

Please use /// comments so doxygen picks it up.  Please rename this to getLandingPadInst


llvm::BasicBlock should probably get an "isLandingPad()" and getLandingPadInst() helper methods, which are  isa<<LandingPadInst>(getFirstNonPHI()) (and dyn_cast).


+++ include/llvm-c/Core.h	(working copy)

+typedef enum {
+  LLVMCatch,              /**< A catch clause   */
+  LLVMFilter              /**< A filter clause  */
+} LLVMLandingPadClauseTy;

Like LLVMRealPredicate, you should mangle the enum name into the enumerators.


In Verifier:
@@ -1361,7 +1373,7 @@
   Assert1(Ordering == Acquire || Ordering == Release ||
           Ordering == AcquireRelease || Ordering == SequentiallyConsistent,
           "fence instructions may only have "
-          " acquire, release, acq_rel, or seq_cst ordering.", &FI);
+          "acquire, release, acq_rel, or seq_cst ordering.", &FI);
   visitInstruction(FI);
 }


Please commit this separately.

+  // The landingpad instruction must be the first non-PHI instruction in the
+  // block.
+  BasicBlock::iterator I = BB->begin(), E = BB->end();
+  while (I != E && isa<PHINode>(I))
+    ++I;
+  Assert1(I != E && isa<LandingPadInst>(I) && I == LPI,
+          "LandingPadInst not the first non-PHI instruction in the block.",
+          &LPI);

Use "LPI->getParent()->getLandingPadInst() == &LPI)



Unrelated, but noticed while reviewing the patch, the grammar in LangRef.html is actually:

<resultval> = landingpad <somety> personality <type> <pers_fn> <clause>+
<resultval> = landingpad <somety> personality <type> <pers_fn> cleanup <clause>*

Please fix it.

Please resend with the grammar fix.  I can't review most of this patch without that, and the ripples through the rest of the code, fixed.

Thanks Bill!

-Chris