Address code review comments

Author: AmrDeveloper

clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h

@@ -389,25 +389,6 @@ class CIRBaseBuilderTy : public mlir::OpBuilder {
     return createCallOp(loc, callee, cir::VoidType(), operands, attrs);
   }
 
-  cir::CallOp createTryCallOp(
-      mlir::Location loc, mlir::SymbolRefAttr callee = mlir::SymbolRefAttr(),
-      mlir::Type returnType = cir::VoidType(),
-      mlir::ValueRange operands = mlir::ValueRange(),
-      [[maybe_unused]] cir::SideEffect sideEffect = cir::SideEffect::All) {
-    assert(!cir::MissingFeatures::opCallCallConv());
-    assert(!cir::MissingFeatures::opCallSideEffect());
-    return createCallOp(loc, callee, returnType, operands);
-  }
-
-  cir::CallOp createTryCallOp(
-      mlir::Location loc, cir::FuncOp callee, mlir::ValueRange operands,
-      [[maybe_unused]] cir::SideEffect sideEffect = cir::SideEffect::All) {
-    assert(!cir::MissingFeatures::opCallCallConv());
-    assert(!cir::MissingFeatures::opCallSideEffect());
-    return createTryCallOp(loc, mlir::SymbolRefAttr::get(callee),
-                           callee.getFunctionType().getReturnType(), operands);
-  }
-
   //===--------------------------------------------------------------------===//
   // Cast/Conversion Operators
   //===--------------------------------------------------------------------===//

clang/lib/CIR/CodeGen/CIRGenCall.cpp

@@ -472,11 +472,11 @@ emitCallLikeOp(CIRGenFunction &cgf, mlir::Location callLoc,
   assert(!cir::MissingFeatures::opCallSurroundingTry());
 
   if (isInvoke) {
-    // This call can throw, few options:
-    //  - If this call does not have an associated cir.try, use the
-    //    one provided by InvokeDest,
-    //  - User written try/catch clauses require calls to handle
-    //    exceptions under cir.try.
+    // This call may throw and requires catch and/or cleanup handling.
+    // If this call does not appear within the `try` region of an existing
+    // TryOp, we must create a synthetic TryOp to contain the call. This
+    // happens when a call that may throw appears within a cleanup
+    // scope.
 
     // In OG, we build the landing pad for this scope. In CIR, we emit a
     // synthetic cir.try because this didn't come from code generating from a
@@ -501,7 +501,7 @@ emitCallLikeOp(CIRGenFunction &cgf, mlir::Location callLoc,
     }
 
     callOpWithExceptions =
-        builder.createTryCallOp(callLoc, directFuncOp, cirCallArgs);
+        builder.createCallOp(callLoc, directFuncOp, cirCallArgs);
 
     (void)cgf.getInvokeDest(tryOp);
 

clang/lib/CIR/CodeGen/CIRGenCleanup.cpp

@@ -188,7 +188,7 @@ void EHScopeStack::popCleanup() {
   }
 }
 
-bool EHScopeStack::requiresLandingPad() const {
+bool EHScopeStack::requiresCatchOrCleanup() const {
   for (stable_iterator si = getInnermostEHScope(); si != stable_end();) {
     // TODO(cir): Skip lifetime markers.
     assert(!cir::MissingFeatures::emitLifetimeMarkers());

clang/lib/CIR/CodeGen/CIRGenCleanup.h

@@ -38,7 +38,7 @@ class EHScope {
   };
   enum { NumCommonBits = 3 };
 
-  bool isScopeMayThrow;
+  bool scopeMayThrow;
 
 protected:
   class CatchBitFields {
@@ -94,10 +94,10 @@ class EHScope {
     // Traditional LLVM codegen also checks for `!block->use_empty()`, but
     // in CIRGen the block content is not important, just used as a way to
     // signal `hasEHBranches`.
-    return isScopeMayThrow;
+    return scopeMayThrow;
   }
 
-  void setMayThrow(bool mayThrow) { isScopeMayThrow = mayThrow; }
+  void setMayThrow(bool mayThrow) { scopeMayThrow = mayThrow; }
 
   EHScopeStack::stable_iterator getEnclosingEHScope() const {
     return enclosingEHScope;

clang/lib/CIR/CodeGen/CIRGenException.cpp

@@ -355,33 +355,6 @@ void CIRGenFunction::enterCXXTryStmt(const CXXTryStmt &s, cir::TryOp tryOp,
   }
 }
 
-/// Emit the structure of the dispatch block for the given catch scope.
-/// It is an invariant that the dispatch block already exists.
-static void emitCatchDispatchBlock(CIRGenFunction &cgf,
-                                   EHCatchScope &catchScope, cir::TryOp tryOp) {
-  if (EHPersonality::get(cgf).isWasmPersonality()) {
-    cgf.cgm.errorNYI("emitCatchDispatchBlock: WASM personality");
-    return;
-  }
-
-  if (EHPersonality::get(cgf).usesFuncletPads()) {
-    cgf.cgm.errorNYI("emitCatchDispatchBlock: usesFuncletPads");
-    return;
-  }
-
-  assert(catchScope.mayThrow() &&
-         "Expected catchScope that may throw exception");
-
-  // If there's only a single catch-all, getEHDispatchBlock returned
-  // that catch-all as the dispatch block.
-  if (catchScope.getNumHandlers() == 1 &&
-      catchScope.getHandler(0).isCatchAll()) {
-    return;
-  }
-
-  cgf.cgm.errorNYI("emitCatchDispatchBlock: non-catch all handler");
-}
-
 void CIRGenFunction::exitCXXTryStmt(const CXXTryStmt &s, bool isFnTryBlock) {
   unsigned numHandlers = s.getNumHandlers();
   EHCatchScope &catchScope = cast<EHCatchScope>(*ehStack.begin());
@@ -410,9 +383,6 @@ void CIRGenFunction::exitCXXTryStmt(const CXXTryStmt &s, bool isFnTryBlock) {
     return;
   }
 
-  // Emit the structure of the EH dispatch for this catch.
-  emitCatchDispatchBlock(*this, catchScope, tryOp);
-
   // Copy the handler blocks off before we pop the EH stack.  Emitting
   // the handlers might scribble on this memory.
   SmallVector<EHCatchScope::Handler, 8> handlers(
@@ -490,8 +460,8 @@ void CIRGenFunction::exitCXXTryStmt(const CXXTryStmt &s, bool isFnTryBlock) {
   assert(!cir::MissingFeatures::incrementProfileCounter());
 }
 
-mlir::Operation *CIRGenFunction::emitLandingPad(cir::TryOp tryOp) {
-  assert(ehStack.requiresLandingPad());
+mlir::Operation *CIRGenFunction::populateCatchHandlers(cir::TryOp tryOp) {
+  assert(ehStack.requiresCatchOrCleanup());
   assert(!cgm.getLangOpts().IgnoreExceptions &&
          "LandingPad should not be emitted when -fignore-exceptions are in "
          "effect.");
@@ -551,17 +521,17 @@ mlir::Operation *CIRGenFunction::emitLandingPad(cir::TryOp tryOp) {
         if (handler.isCatchAll()) {
           assert(!hasCatchAll);
           hasCatchAll = true;
-          goto done;
+          break;
         }
 
         cgm.errorNYI("emitLandingPad: non catch-all");
         return {};
       }
 
-      goto done;
+      if (hasCatchAll)
+        break;
     }
 
-  done:
     if (hasCatchAll) {
       handlerAttrs.push_back(cir::CatchAllAttr::get(&getMLIRContext()));
     } else {
@@ -576,20 +546,19 @@ mlir::Operation *CIRGenFunction::emitLandingPad(cir::TryOp tryOp) {
 
   // In traditional LLVM codegen. this tells the backend how to generate the
   // landing pad by generating a branch to the dispatch block. In CIR,
-  // getEHDispatchBlock is used to populate blocks for later filing during
+  // this is used to populate blocks for later filing during
   // cleanup handling.
-  (void)getEHDispatchBlock(ehStack.getInnermostEHScope(), tryOp);
+  (void)populateEHCatchRegions(ehStack.getInnermostEHScope(), tryOp);
 
   return tryOp;
 }
 
 // Differently from LLVM traditional codegen, there are no dispatch blocks
 // to look at given cir.try_call does not jump to blocks like invoke does.
-// However, we keep this around since other parts of CIRGen use
-// getCachedEHDispatchBlock to infer state.
+// However.
 mlir::Block *
-CIRGenFunction::getEHDispatchBlock(EHScopeStack::stable_iterator scope,
-                                   cir::TryOp tryOp) {
+CIRGenFunction::populateEHCatchRegions(EHScopeStack::stable_iterator scope,
+                                       cir::TryOp tryOp) {
   if (EHPersonality::get(*this).usesFuncletPads()) {
     cgm.errorNYI("getEHDispatchBlock: usesFuncletPads");
     return {};
@@ -648,7 +617,7 @@ CIRGenFunction::getEHDispatchBlock(EHScopeStack::stable_iterator scope,
 }
 
 bool CIRGenFunction::isInvokeDest() {
-  if (!ehStack.requiresLandingPad())
+  if (!ehStack.requiresCatchOrCleanup())
     return false;
 
   // If exceptions are disabled/ignored and SEH is not in use, then there is no
@@ -669,7 +638,7 @@ bool CIRGenFunction::isInvokeDest() {
 }
 
 mlir::Operation *CIRGenFunction::getInvokeDestImpl(cir::TryOp tryOp) {
-  assert(ehStack.requiresLandingPad());
+  assert(ehStack.requiresCatchOrCleanup());
   assert(!ehStack.empty());
 
   // TODO(cir): add personality function
@@ -681,7 +650,7 @@ mlir::Operation *CIRGenFunction::getInvokeDestImpl(cir::TryOp tryOp) {
   if (personality.usesFuncletPads()) {
     cgm.errorNYI("getInvokeDestImpl: usesFuncletPads");
   } else {
-    lp = emitLandingPad(tryOp);
+    lp = populateCatchHandlers(tryOp);
   }
 
   return lp;

clang/lib/CIR/CodeGen/CIRGenFunction.h

@@ -916,16 +916,16 @@ class CIRGenFunction : public CIRGenTypeCache {
     return false;
   }
 
-  mlir::Block *getEHDispatchBlock(EHScopeStack::stable_iterator scope,
-                                  cir::TryOp tryOp);
+  mlir::Block *populateEHCatchRegions(EHScopeStack::stable_iterator scope,
+                                      cir::TryOp tryOp);
 
   /// The cleanup depth enclosing all the cleanups associated with the
   /// parameters.
   EHScopeStack::stable_iterator prologueCleanupDepth;
 
   mlir::Operation *getInvokeDestImpl(cir::TryOp tryOp);
   mlir::Operation *getInvokeDest(cir::TryOp tryOp) {
-    if (!ehStack.requiresLandingPad())
+    if (!ehStack.requiresCatchOrCleanup())
       return nullptr;
     // Return the respective cir.try, this can be used to compute
     // any other relevant information.
@@ -1604,7 +1604,7 @@ class CIRGenFunction : public CIRGenTypeCache {
   void emitLambdaDelegatingInvokeBody(const CXXMethodDecl *md);
   void emitLambdaStaticInvokeBody(const CXXMethodDecl *md);
 
-  mlir::Operation *emitLandingPad(cir::TryOp tryOp);
+  mlir::Operation *populateCatchHandlers(cir::TryOp tryOp);
 
   mlir::LogicalResult emitIfStmt(const clang::IfStmt &s);
 

clang/lib/CIR/CodeGen/EHScopeStack.h

@@ -217,7 +217,7 @@ class EHScopeStack {
   /// Determines whether the exception-scopes stack is empty.
   bool empty() const { return startOfData == endOfBuffer; }
 
-  bool requiresLandingPad() const;
+  bool requiresCatchOrCleanup() const;
 
   /// Determines whether there are any normal cleanups on the stack.
   bool hasNormalCleanups() const {