llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clangir Author: Andy Kaylor (andykaylor) <details> <summary>Changes</summary> For loops and while loops can create variables in their condition regions that require per-iteration cleanup. The CIR dialect previously had no clean way to represent these cleanups while maintaining a separate condition region for the loop operation. This change introduces an optional cleanup region to these loop ops and updates the relevant region successor handling to reflect the insertion of the cleanup region in the control flow when a non-empty cleanup region is present. The CFG flattening pass will handle routing the control flow through the cleanup region in both the normal and EH unwind cases, but this is not yet implemented. That will be added in a follow-up change, as will creation of the cleanup region when it is needed during IR generation. Assisted-by: Cursor / various models --- Patch is 22.79 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/210212.diff 8 Files Affected: - (modified) clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h (+23) - (modified) clang/include/clang/CIR/Dialect/IR/CIROps.td (+79-7) - (modified) clang/include/clang/CIR/Interfaces/CIRLoopOpInterface.td (+13) - (modified) clang/lib/CIR/Dialect/IR/CIRDialect.cpp (+34-2) - (modified) clang/lib/CIR/Interfaces/CIRLoopOpInterface.cpp (+33-18) - (added) clang/test/CIR/IR/invalid-loop-cleanup.cir (+81) - (added) clang/test/CIR/IR/loop-cleanup.cir (+114) - (modified) clang/unittests/CIR/ControlFlowTest.cpp (+70) ``````````diff diff --git a/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h b/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h index f8a3aca76f102..425fa68ad157e 100644 --- a/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h +++ b/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h @@ -267,6 +267,17 @@ class CIRBaseBuilderTy : public mlir::OpBuilder { return cir::WhileOp::create(*this, loc, condBuilder, bodyBuilder); } + /// Create a while operation with a per-iteration cleanup region. + cir::WhileOp createWhile( + mlir::Location loc, + llvm::function_ref<void(mlir::OpBuilder &, mlir::Location)> condBuilder, + llvm::function_ref<void(mlir::OpBuilder &, mlir::Location)> bodyBuilder, + llvm::function_ref<void(mlir::OpBuilder &, mlir::Location)> cleanupBuilder, + cir::CleanupKind cleanupKind) { + return cir::WhileOp::create(*this, loc, condBuilder, bodyBuilder, + cleanupBuilder, cleanupKind); + } + /// Create a for operation. cir::ForOp createFor( mlir::Location loc, @@ -277,6 +288,18 @@ class CIRBaseBuilderTy : public mlir::OpBuilder { stepBuilder); } + /// Create a for operation with a per-iteration cleanup region. + cir::ForOp createFor( + mlir::Location loc, + llvm::function_ref<void(mlir::OpBuilder &, mlir::Location)> condBuilder, + llvm::function_ref<void(mlir::OpBuilder &, mlir::Location)> bodyBuilder, + llvm::function_ref<void(mlir::OpBuilder &, mlir::Location)> stepBuilder, + llvm::function_ref<void(mlir::OpBuilder &, mlir::Location)> cleanupBuilder, + cir::CleanupKind cleanupKind) { + return cir::ForOp::create(*this, loc, condBuilder, bodyBuilder, stepBuilder, + cleanupBuilder, cleanupKind); + } + /// Create a break operation. cir::BreakOp createBreak(mlir::Location loc) { return cir::BreakOp::create(*this, loc); diff --git a/clang/include/clang/CIR/Dialect/IR/CIROps.td b/clang/include/clang/CIR/Dialect/IR/CIROps.td index 762ef56248e4c..976b173db8f94 100644 --- a/clang/include/clang/CIR/Dialect/IR/CIROps.td +++ b/clang/include/clang/CIR/Dialect/IR/CIROps.td @@ -2200,15 +2200,24 @@ class CIR_WhileOpBase<string mnemonic> : CIR_LoopOpBase<mnemonic> { } def CIR_WhileOp : CIR_WhileOpBase<"while"> { - let regions = (region MinSizedRegion<1>:$cond, MinSizedRegion<1>:$body); - let assemblyFormat = "$cond `do` $body attr-dict"; + let arguments = (ins OptionalAttr<CIR_CleanupKindAttr>:$cleanupKind); + let regions = (region MinSizedRegion<1>:$cond, MinSizedRegion<1>:$body, + MaxSizedRegion<1>:$cleanup); + let assemblyFormat = [{ + $cond `do` $body (`cleanup` $cleanupKind $cleanup^)? attr-dict + }]; let description = [{ - Represents a C/C++ while loop. It consists of two regions: + Represents a C/C++ while loop. It consists of two or three regions: - `cond`: single block region with the loop's condition. Should be terminated with a `cir.condition` operation. - `body`: contains the loop body and an arbitrary number of blocks. + - `cleanup`: optional region that runs on every per-iteration exit edge + (condition-false exit, end-of-iteration, break/continue, and EH unwinding + when the cleanup kind includes EH). This is used to destroy a condition + variable whose lifetime is a single iteration. When present, it carries a + cleanup kind matching `cir.cleanup.scope` (`normal` or `all`). Example: @@ -2223,6 +2232,40 @@ def CIR_WhileOp : CIR_WhileOpBase<"while"> { ``` }]; + let builders = [ + OpBuilder<(ins "BuilderCallbackRef":$condBuilder, + "BuilderCallbackRef":$bodyBuilder, + CArg<"BuilderCallbackRef", "nullptr">:$cleanupBuilder, + CArg<"cir::CleanupKind", "cir::CleanupKind::All">:$cleanupKind), [{ + mlir::OpBuilder::InsertionGuard guard($_builder); + $_builder.createBlock($_state.addRegion()); + condBuilder($_builder, $_state.location); + $_builder.createBlock($_state.addRegion()); + bodyBuilder($_builder, $_state.location); + mlir::Region *cleanupRegion = $_state.addRegion(); + if (cleanupBuilder) { + $_state.addAttribute( + getCleanupKindAttrName($_state.name), + cir::CleanupKindAttr::get($_builder.getContext(), cleanupKind)); + $_builder.createBlock(cleanupRegion); + cleanupBuilder($_builder, $_state.location); + } + }]> + ]; + + let extraClassDeclaration = [{ + mlir::Region *maybeGetCleanup() { + return getCleanup().empty() ? nullptr : &getCleanup(); + } + llvm::SmallVector<mlir::Region *> getRegionsInExecutionOrder() { + llvm::SmallVector<mlir::Region *> regions{&getCond(), &getBody()}; + if (mlir::Region *cleanup = maybeGetCleanup()) + regions.push_back(cleanup); + return regions; + } + }]; + + let hasVerifier = 1; let hasLLVMLowering = false; } @@ -2261,12 +2304,18 @@ def CIR_DoWhileOp : CIR_WhileOpBase<"do"> { def CIR_ForOp : CIR_LoopOpBase<"for"> { let summary = "C/C++ for loop counterpart"; let description = [{ - Represents a C/C++ for loop. It consists of three regions: + Represents a C/C++ for loop. It consists of three or four regions: - `cond`: single block region with the loop's condition. Should be terminated with a `cir.condition` operation. - `body`: contains the loop body and an arbitrary number of blocks. - `step`: single block region with the loop's step. + - `cleanup`: optional region that runs on every per-iteration exit edge + (condition-false exit, end-of-iteration after the step, break/continue, + and EH unwinding when the cleanup kind includes EH). This is used to + destroy a condition variable whose lifetime is a single iteration. When + present, it carries a cleanup kind matching `cir.cleanup.scope` (`normal` + or `all`). Example: @@ -2283,20 +2332,25 @@ def CIR_ForOp : CIR_LoopOpBase<"for"> { ``` }]; + let arguments = (ins OptionalAttr<CIR_CleanupKindAttr>:$cleanupKind); let regions = (region MinSizedRegion<1>:$cond, MinSizedRegion<1>:$body, - MinSizedRegion<1>:$step); + MinSizedRegion<1>:$step, + MaxSizedRegion<1>:$cleanup); let assemblyFormat = [{ `:` `cond` $cond `body` $body `step` $step + (`cleanup` $cleanupKind $cleanup^)? attr-dict }]; let builders = [ OpBuilder<(ins "llvm::function_ref<void(mlir::OpBuilder &, mlir::Location)>":$condBuilder, "llvm::function_ref<void(mlir::OpBuilder &, mlir::Location)>":$bodyBuilder, - "llvm::function_ref<void(mlir::OpBuilder &, mlir::Location)>":$stepBuilder), [{ + "llvm::function_ref<void(mlir::OpBuilder &, mlir::Location)>":$stepBuilder, + CArg<"llvm::function_ref<void(mlir::OpBuilder &, mlir::Location)>", "nullptr">:$cleanupBuilder, + CArg<"cir::CleanupKind", "cir::CleanupKind::All">:$cleanupKind), [{ mlir::OpBuilder::InsertionGuard guard($_builder); // Build condition region. @@ -2310,16 +2364,34 @@ def CIR_ForOp : CIR_LoopOpBase<"for"> { // Build step region. $_builder.createBlock($_state.addRegion()); stepBuilder($_builder, $_state.location); + + // Build optional cleanup region. + mlir::Region *cleanupRegion = $_state.addRegion(); + if (cleanupBuilder) { + $_state.addAttribute( + getCleanupKindAttrName($_state.name), + cir::CleanupKindAttr::get($_builder.getContext(), cleanupKind)); + $_builder.createBlock(cleanupRegion); + cleanupBuilder($_builder, $_state.location); + } }]> ]; let extraClassDeclaration = [{ mlir::Region *maybeGetStep() { return &getStep(); } + mlir::Region *maybeGetCleanup() { + return getCleanup().empty() ? nullptr : &getCleanup(); + } llvm::SmallVector<mlir::Region *> getRegionsInExecutionOrder() { - return llvm::SmallVector<mlir::Region *, 3>{&getCond(), &getBody(), &getStep()}; + llvm::SmallVector<mlir::Region *> regions{&getCond(), &getBody(), + &getStep()}; + if (mlir::Region *cleanup = maybeGetCleanup()) + regions.push_back(cleanup); + return regions; } }]; + let hasVerifier = 1; let hasLLVMLowering = false; } diff --git a/clang/include/clang/CIR/Interfaces/CIRLoopOpInterface.td b/clang/include/clang/CIR/Interfaces/CIRLoopOpInterface.td index 71e3d934b9da1..01b78a2a8b413 100644 --- a/clang/include/clang/CIR/Interfaces/CIRLoopOpInterface.td +++ b/clang/include/clang/CIR/Interfaces/CIRLoopOpInterface.td @@ -45,6 +45,19 @@ def LoopOpInterface : OpInterface<"LoopOpInterface", [ /*methodBody=*/"", /*defaultImplementation=*/"return nullptr;" >, + InterfaceMethod<[{ + Returns a pointer to the loop's per-iteration cleanup region, or + nullptr if the loop has no (non-empty) cleanup region. When present, + this region runs on every per-iteration exit edge (condition-false + exit, end-of-iteration, break/continue, and EH unwinding when the + cleanup kind includes EH). + }], + /*retTy=*/"mlir::Region *", + /*methodName=*/"maybeGetCleanup", + /*args=*/(ins), + /*methodBody=*/"", + /*defaultImplementation=*/"return nullptr;" + >, InterfaceMethod<[{ Returns the first region to be executed in the loop. }], diff --git a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp index c519229a02f26..057ec610bdc8a 100644 --- a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp +++ b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp @@ -502,10 +502,14 @@ void cir::ConditionOp::getSuccessorRegions( // TODO(cir): The condition value may be folded to a constant, narrowing // down its list of possible successors. - // Parent is a loop: condition may branch to the body or to the parent op. + // Parent is a loop: condition may branch to the body, or on the false edge + // to the per-iteration cleanup region if present, otherwise to the parent op. if (auto loopOp = dyn_cast<LoopOpInterface>(getOperation()->getParentOp())) { regions.emplace_back(&loopOp.getBody()); - regions.emplace_back(getOperation()); + if (mlir::Region *cleanup = loopOp.maybeGetCleanup()) + regions.emplace_back(cleanup); + else + regions.emplace_back(getOperation()); return; } @@ -533,6 +537,34 @@ LogicalResult cir::ConditionOp::verify() { return success(); } +//===----------------------------------------------------------------------===// +// WhileOp & ForOp +//===----------------------------------------------------------------------===// + +template <typename LoopOpTy> +static LogicalResult verifyLoopCleanup(LoopOpTy op) { + std::optional<cir::CleanupKind> cleanupKind = op.getCleanupKind(); + + // The cleanup kind attribute must be present exactly when a (non-empty) + // cleanup region is present. + if (cleanupKind.has_value() == op.getCleanup().empty()) + return op.emitOpError("cleanup kind must be present if and only if the " + "cleanup region is non-empty"); + + // A loop's per-iteration cleanup runs on every normal exit edge (loop exit, + // end of iteration, break/continue), so an EH-only cleanup is meaningless. + // Only 'normal' (exceptions disabled) and 'all' (normal + EH unwind) apply. + if (cleanupKind == cir::CleanupKind::EH) + return op.emitOpError("loop cleanup kind must be 'normal' or 'all', " + "not 'eh'"); + + return success(); +} + +LogicalResult cir::WhileOp::verify() { return verifyLoopCleanup(*this); } + +LogicalResult cir::ForOp::verify() { return verifyLoopCleanup(*this); } + //===----------------------------------------------------------------------===// // ConstantOp //===----------------------------------------------------------------------===// diff --git a/clang/lib/CIR/Interfaces/CIRLoopOpInterface.cpp b/clang/lib/CIR/Interfaces/CIRLoopOpInterface.cpp index c2a6ef81f17b8..1dad9acb62e48 100644 --- a/clang/lib/CIR/Interfaces/CIRLoopOpInterface.cpp +++ b/clang/lib/CIR/Interfaces/CIRLoopOpInterface.cpp @@ -28,25 +28,48 @@ void LoopOpInterface::getLoopOpSuccessorRegions( mlir::Region *parentRegion = point.getTerminatorPredecessorOrNull()->getParentRegion(); - // Branching from condition: go to body or exit. + mlir::Region *step = op.maybeGetStep(); + mlir::Region *cleanup = op.maybeGetCleanup(); + + // Branching from condition: go to body, or (on the false edge) route + // through the cleanup region if present, otherwise exit the loop. if (&op.getCond() == parentRegion) { - regions.emplace_back(op); + if (cleanup) + regions.emplace_back(cleanup); + else + regions.emplace_back(op); regions.emplace_back(&op.getBody()); return; } - // Branching from body: go to step (for) or condition. + // Branching from body: go to step (for), otherwise route through the + // cleanup region if present, otherwise go back to the condition. if (&op.getBody() == parentRegion) { // FIXME(cir): Should we consider break/continue statements here? - mlir::Region *afterBody = - (op.maybeGetStep() ? op.maybeGetStep() : &op.getCond()); - regions.emplace_back(afterBody); + if (step) + regions.emplace_back(step); + else if (cleanup) + regions.emplace_back(cleanup); + else + regions.emplace_back(&op.getCond()); + return; + } + + // Branching from step: route through the cleanup region if present, + // otherwise go back to the condition. + if (step == parentRegion) { + if (cleanup) + regions.emplace_back(cleanup); + else + regions.emplace_back(&op.getCond()); return; } - // Branching from step: go to condition. - if (op.maybeGetStep() == parentRegion) { + // Branching from cleanup: either loop back to the condition (normal + // end-of-iteration) or exit the loop (condition-false edge). + if (cleanup == parentRegion) { regions.emplace_back(&op.getCond()); + regions.emplace_back(op); return; } @@ -58,16 +81,8 @@ LoopOpInterface::getLoopOpSuccessorInputs(LoopOpInterface op, mlir::RegionSuccessor successor) { if (successor.isOperation()) return op->getResults(); - if (successor == &op.getEntry()) - return op.getEntry().getArguments(); - if (successor == &op.getBody()) - return op.getBody().getArguments(); - mlir::Region *afterBody = - (op.maybeGetStep() ? op.maybeGetStep() : &op.getCond()); - if (successor == afterBody) - return afterBody->getArguments(); - if (successor == &op.getCond()) - return op.getCond().getArguments(); + if (mlir::Region *region = successor.getSuccessor()) + return region->getArguments(); llvm_unreachable("invalid region successor"); } diff --git a/clang/test/CIR/IR/invalid-loop-cleanup.cir b/clang/test/CIR/IR/invalid-loop-cleanup.cir new file mode 100644 index 0000000000000..c8efedb5ddbd6 --- /dev/null +++ b/clang/test/CIR/IR/invalid-loop-cleanup.cir @@ -0,0 +1,81 @@ +// RUN: cir-opt %s -verify-diagnostics -split-input-file + +!s32i = !cir.int<s, 32> + +cir.func private @dtor(!cir.ptr<!s32i>) + +cir.func @while_cleanup_missing_kind(%cond : !cir.bool, %s : !cir.ptr<!s32i>) { + cir.while { + cir.condition(%cond) + } do { + cir.yield + // expected-error @below {{expected valid keyword or string}} + // expected-error @below {{failed to parse CIR_CleanupKindAttr}} + } cleanup { + cir.call @dtor(%s) : (!cir.ptr<!s32i>) -> () + cir.yield + } + cir.return +} + +// ----- + +!s32i = !cir.int<s, 32> + +cir.func private @dtor(!cir.ptr<!s32i>) + +cir.func @for_cleanup_missing_kind(%cond : !cir.bool, %s : !cir.ptr<!s32i>) { + cir.for : cond { + cir.condition(%cond) + } body { + cir.yield + } step { + cir.yield + // expected-error @below {{expected valid keyword or string}} + // expected-error @below {{failed to parse CIR_CleanupKindAttr}} + } cleanup { + cir.call @dtor(%s) : (!cir.ptr<!s32i>) -> () + cir.yield + } + cir.return +} + +// ----- + +!s32i = !cir.int<s, 32> + +cir.func private @dtor(!cir.ptr<!s32i>) + +cir.func @while_cleanup_eh(%cond : !cir.bool, %s : !cir.ptr<!s32i>) { + // expected-error @below {{loop cleanup kind must be 'normal' or 'all', not 'eh'}} + cir.while { + cir.condition(%cond) + } do { + cir.yield + } cleanup eh { + cir.call @dtor(%s) : (!cir.ptr<!s32i>) -> () + cir.yield + } + cir.return +} + +// ----- + +!s32i = !cir.int<s, 32> + +cir.func private @dtor(!cir.ptr<!s32i>) + +cir.func @for_cleanup_eh(%cond : !cir.bool, %s : !cir.ptr<!s32i>) { + // expected-error @below {{loop cleanup kind must be 'normal' or 'all', not 'eh'}} + cir.for : cond { + cir.condition(%cond) + } body { + cir.yield + } step { + cir.yield + } cleanup eh { + cir.call @dtor(%s) : (!cir.ptr<!s32i>) -> () + cir.yield + } + cir.return +} diff --git a/clang/test/CIR/IR/loop-cleanup.cir b/clang/test/CIR/IR/loop-cleanup.cir new file mode 100644 index 0000000000000..bdc896a7ba822 --- /dev/null +++ b/clang/test/CIR/IR/loop-cleanup.cir @@ -0,0 +1,114 @@ +// RUN: cir-opt %s --verify-roundtrip | FileCheck %s + +!s32i = !cir.int<s, 32> + +module { + cir.func private @dtor(!cir.ptr<!s32i>) + + // A while loop with a per-iteration cleanup region that runs on both the + // normal and EH exit edges. + cir.func @while_cleanup_all(%cond : !cir.bool, %s : !cir.ptr<!s32i>) { + cir.while { + cir.condition(%cond) + } do { + cir.yield + } cleanup all { + cir.call @dtor(%s) : (!cir.ptr<!s32i>) -> () + cir.yield + } + cir.return + } + + // CHECK-LABEL: cir.func @while_cleanup_all + // CHECK: cir.while { + // CHECK: cir.condition(%[[COND:.*]]) + // CHECK: } do { + // CHECK: cir.yield + // CHECK: } cleanup all { + // CHECK: cir.call @dtor + // CHECK: cir.yield + // CHECK: } + + // A while loop with a normal-only cleanup region (exceptions disabled). + cir.func @while_cleanup_normal(%cond : !cir.bool, %s : !cir.ptr<!s32i>) { + cir.while { + cir.condition(%cond) + } do { + cir.yield + } cleanup normal { + cir.call @dtor(%s) : (!cir.ptr<!s32i>) -> () + cir.yield + } + cir.return + } + + // CHECK-LABEL: cir.func @while_cleanup_normal + // CHECK: } cleanup normal { + // CHECK: cir.call @dtor + // CHECK: } + + // A while loop without a cleanup region still prints as before. + cir.func @while_no_cleanup(%cond : !cir.bool) { + cir.while { + cir.condition(%cond) + } do { + cir.yield + } + cir.return + } + + // CHECK-LABEL: cir.func @while_no_cleanup + // CHECK: cir.while { + // CHECK: cir.condition + // CHECK: } do { + // CHECK: cir.yield + // CHECK-NOT: cleanup + + // A for loop with a per-iteration cleanup region after the step region. + cir.func @for_cleanup_all(%cond : !cir.bool, %s : !cir.ptr<!s32i>) { + cir.for : cond { + cir.condition(%cond) + } body { + cir.yield + } step { + cir.yield + } cleanup all { + cir.call @dtor(%s) : (!cir.ptr<!s32i>) -> () + cir.yield + } + cir.return + } + + // CHECK-LABEL: cir.func @for_cleanup_all + // CHECK: cir.for : cond { + // CHECK: cir.condition(%[[FCOND:.*]]) + // CHECK: } body { + // CHECK: cir.yield + // CHECK: } step { + // CHECK: cir.yield + // CHECK: } cleanup all { + // CHECK: cir.call @dtor + // CHECK: cir.yield + // CHECK: } + + // A for loop without a cleanup region still prints as before. + cir.func @for_no_cleanup(%cond : !cir.bool) { + cir.for : cond { + cir.condition(%cond) + } body { + cir.yield + } step { + cir.yield + } + cir.return + } + + // CHECK-LABEL: cir.func @for_no_cleanup + // CHECK: cir.for : cond { + // CHECK: cir.condition + // CHEC... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/210212 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
