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,10 +501,9 @@ emitCallLikeOp(CIRGenFunction &cgf, mlir::Location callLoc,
     }
 
     callOpWithExceptions =
-        builder.createTryCallOp(callLoc, directFuncOp, cirCallArgs);
-
-    (void)cgf.getInvokeDest(tryOp);
+        builder.createCallOp(callLoc, directFuncOp, cirCallArgs);
 
+    cgf.populateCatchHandlersIfRequired(tryOp);
     return callOpWithExceptions;
   }
 
@@ -668,7 +667,7 @@ RValue CIRGenFunction::emitCall(const CIRGenFunctionInfo &funcInfo,
   // TODO(cir): check for MSVCXXPersonality
   // TODO(cir): Create NoThrowAttr
   bool cannotThrow = attrs.getNamed("nothrow").has_value();
-  bool isInvoke = !cannotThrow && isInvokeDest();
+  bool isInvoke = !cannotThrow && isCatchOrCleanupRequired();
 
   mlir::Location callLoc = loc;
   cir::CIRCallOpInterface theCall =

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());
+void CIRGenFunction::populateCatchHandlers(cir::TryOp tryOp) {
+  assert(ehStack.requiresCatchOrCleanup());
   assert(!cgm.getLangOpts().IgnoreExceptions &&
          "LandingPad should not be emitted when -fignore-exceptions are in "
          "effect.");
@@ -500,7 +470,7 @@ mlir::Operation *CIRGenFunction::emitLandingPad(cir::TryOp tryOp) {
   switch (innermostEHScope.getKind()) {
   case EHScope::Terminate:
     cgm.errorNYI("emitLandingPad: terminate");
-    return {};
+    return;
 
   case EHScope::Catch:
   case EHScope::Cleanup:
@@ -523,17 +493,17 @@ mlir::Operation *CIRGenFunction::emitLandingPad(cir::TryOp tryOp) {
       switch (i->getKind()) {
       case EHScope::Cleanup: {
         cgm.errorNYI("emitLandingPad: Cleanup");
-        return {};
+        return;
       }
 
       case EHScope::Filter: {
         cgm.errorNYI("emitLandingPad: Filter");
-        return {};
+        return;
       }
 
       case EHScope::Terminate: {
         cgm.errorNYI("emitLandingPad: Terminate");
-        return {};
+        return;
       }
 
       case EHScope::Catch:
@@ -551,22 +521,22 @@ mlir::Operation *CIRGenFunction::emitLandingPad(cir::TryOp tryOp) {
         if (handler.isCatchAll()) {
           assert(!hasCatchAll);
           hasCatchAll = true;
-          goto done;
+          break;
         }
 
         cgm.errorNYI("emitLandingPad: non catch-all");
-        return {};
+        return;
       }
 
-      goto done;
+      if (hasCatchAll)
+        break;
     }
 
-  done:
     if (hasCatchAll) {
       handlerAttrs.push_back(cir::CatchAllAttr::get(&getMLIRContext()));
     } else {
       cgm.errorNYI("emitLandingPad: non catch-all");
-      return {};
+      return;
     }
 
     // Add final array of clauses into TryOp.
@@ -576,23 +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);
-
-  return tryOp;
+  populateEHCatchRegions(ehStack.getInnermostEHScope(), 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.
-mlir::Block *
-CIRGenFunction::getEHDispatchBlock(EHScopeStack::stable_iterator scope,
-                                   cir::TryOp tryOp) {
+// However.
+void CIRGenFunction::populateEHCatchRegions(EHScopeStack::stable_iterator scope,
+                                            cir::TryOp tryOp) {
   if (EHPersonality::get(*this).usesFuncletPads()) {
     cgm.errorNYI("getEHDispatchBlock: usesFuncletPads");
-    return {};
+    return;
   }
 
   // Otherwise, we should look at the actual scope.
@@ -606,7 +572,7 @@ CIRGenFunction::getEHDispatchBlock(EHScopeStack::stable_iterator scope,
     // - Update the map to enqueue new dispatchBlock to also get a cleanup. See
     // code at the end of the function.
     cgm.errorNYI("getEHDispatchBlock: mayThrow & tryOp");
-    return {};
+    return;
   }
 
   if (!mayThrow) {
@@ -621,34 +587,33 @@ CIRGenFunction::getEHDispatchBlock(EHScopeStack::stable_iterator scope,
         break;
       }
       cgm.errorNYI("getEHDispatchBlock: mayThrow non-catch all");
-      return {};
+      return;
     }
     case EHScope::Cleanup: {
       cgm.errorNYI("getEHDispatchBlock: mayThrow & cleanup");
-      return {};
+      return;
     }
     case EHScope::Filter: {
       cgm.errorNYI("getEHDispatchBlock: mayThrow & Filter");
-      return {};
+      return;
     }
     case EHScope::Terminate: {
       cgm.errorNYI("getEHDispatchBlock: mayThrow & Terminate");
-      return {};
+      return;
     }
     }
   }
 
   if (originalBlock) {
     cgm.errorNYI("getEHDispatchBlock: originalBlock");
-    return {};
+    return;
   }
 
   ehScope.setMayThrow(mayThrow);
-  return {};
 }
 
-bool CIRGenFunction::isInvokeDest() {
-  if (!ehStack.requiresLandingPad())
+bool CIRGenFunction::isCatchOrCleanupRequired() {
+  if (!ehStack.requiresCatchOrCleanup())
     return false;
 
   // If exceptions are disabled/ignored and SEH is not in use, then there is no
@@ -668,21 +633,17 @@ bool CIRGenFunction::isInvokeDest() {
   return true;
 }
 
-mlir::Operation *CIRGenFunction::getInvokeDestImpl(cir::TryOp tryOp) {
-  assert(ehStack.requiresLandingPad());
+void CIRGenFunction::populateCatchHandlersIfRequired(cir::TryOp tryOp) {
+  assert(ehStack.requiresCatchOrCleanup());
   assert(!ehStack.empty());
 
   // TODO(cir): add personality function
 
   // CIR does not cache landing pads.
   const EHPersonality &personality = EHPersonality::get(*this);
-
-  mlir::Operation *lp = nullptr;
   if (personality.usesFuncletPads()) {
     cgm.errorNYI("getInvokeDestImpl: usesFuncletPads");
   } else {
-    lp = emitLandingPad(tryOp);
+    populateCatchHandlers(tryOp);
   }
-
-  return lp;
 }

clang/lib/CIR/CodeGen/CIRGenFunction.h

@@ -916,22 +916,15 @@ class CIRGenFunction : public CIRGenTypeCache {
     return false;
   }
 
-  mlir::Block *getEHDispatchBlock(EHScopeStack::stable_iterator scope,
-                                  cir::TryOp tryOp);
+  void 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())
-      return nullptr;
-    // Return the respective cir.try, this can be used to compute
-    // any other relevant information.
-    return getInvokeDestImpl(tryOp);
-  }
-  bool isInvokeDest();
+  bool isCatchOrCleanupRequired();
+  void populateCatchHandlersIfRequired(cir::TryOp tryOp);
 
   /// Takes the old cleanup stack size and emits the cleanup blocks
   /// that have been added.
@@ -1626,7 +1619,7 @@ class CIRGenFunction : public CIRGenTypeCache {
   void emitLambdaDelegatingInvokeBody(const CXXMethodDecl *md);
   void emitLambdaStaticInvokeBody(const CXXMethodDecl *md);
 
-  mlir::Operation *emitLandingPad(cir::TryOp tryOp);
+  void 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 {