Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions clang/include/clang/CIR/Dialect/IR/CIRAttrs.td
Original file line number Diff line number Diff line change
Expand Up @@ -960,4 +960,19 @@ def CIR_TypeInfoAttr : CIR_Attr<"TypeInfo", "typeinfo", [TypedAttrInterface]> {
}];
}

//===----------------------------------------------------------------------===//
// CatchAllAttr & CatchUnwindAttr
//===----------------------------------------------------------------------===//

// Represents the unwind region where unwind continues or
// the program std::terminate's.
def CIR_CatchUnwindAttr : CIR_UnitAttr<"CatchUnwind", "unwind"> {
let storageType = [{ CatchUnwind }];
}

// Represents the catch_all region.
def CIR_CatchAllAttr : CIR_UnitAttr<"CatchAll", "all"> {
let storageType = [{ CatchAllAttr }];
}

#endif // CLANG_CIR_DIALECT_IR_CIRATTRS_TD
63 changes: 61 additions & 2 deletions clang/include/clang/CIR/Dialect/IR/CIROps.td
Original file line number Diff line number Diff line change
Expand Up @@ -631,7 +631,7 @@ def CIR_StoreOp : CIR_Op<"store", [

defvar CIR_ReturnableScopes = [
"FuncOp", "ScopeOp", "IfOp", "SwitchOp", "CaseOp",
"DoWhileOp", "WhileOp", "ForOp"
"DoWhileOp", "WhileOp", "ForOp", "TryOp"
];

def CIR_ReturnOp : CIR_Op<"return", [
Expand Down Expand Up @@ -778,7 +778,7 @@ def CIR_ConditionOp : CIR_Op<"condition", [

defvar CIR_YieldableScopes = [
"ArrayCtor", "ArrayDtor", "CaseOp", "DoWhileOp", "ForOp", "GlobalOp", "IfOp",
"ScopeOp", "SwitchOp", "TernaryOp", "WhileOp"
"ScopeOp", "SwitchOp", "TernaryOp", "WhileOp", "TryOp"
];

def CIR_YieldOp : CIR_Op<"yield", [
Expand Down Expand Up @@ -4296,6 +4296,65 @@ def CIR_AllocExceptionOp : CIR_Op<"alloc.exception"> {
}];
}

//===----------------------------------------------------------------------===//
// TryOp
//===----------------------------------------------------------------------===//

def CIR_TryOp : CIR_Op<"try",[
DeclareOpInterfaceMethods<RegionBranchOpInterface>,
RecursivelySpeculatable, AutomaticAllocationScope, NoRegionArguments
]> {
let summary = "C++ try block";
let description = [{
Holds the lexical scope of `try {}`. Note that resources used on catch
clauses are usually allocated in the same parent as `cir.try`.

`synthetic`: use `cir.try` to represent try/catches not originally
present in the source code (e.g. `g = new Class` under `-fexceptions`).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
present in the source code (e.g. `g = new Class` under `-fexceptions`).
present in the source code. For example, a synthetic `cir.try` region
is created around the constructor call when `operator new` is used
so that the memory allocated will be freed if the constructor throws
an exception.


`cleanup`: signal to targets (LLVM for now) that this try/catch, needs
to specially tag their landing pads as needing "cleanup".
Comment on lines +4315 to +4316
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be described in terms of the semantics of the CIR itself.

Suggested change
`cleanup`: signal to targets (LLVM for now) that this try/catch, needs
to specially tag their landing pads as needing "cleanup".
`cleanup`: indicates that there are cleanups that must be performed
when exiting the try region via exception, even if the exception is not
caught.


Example:

```mlir
%0 = cir.alloc.exception 16 -> !cir.ptr<!some_record>
%1 = cir.get_global @d2 : !cir.ptr<!some_record>
cir.try synthetic cleanup {
cir.call exception @_ZN7test2_DC1ERKS_(%0, %1)
: (!cir.ptr<!some_record>, !cir.ptr<!some_record>) -> () cleanup {
%2 = cir.cast bitcast %0 : !cir.ptr<!some_record> -> !cir.ptr<!void>
cir.free.exception %2
cir.yield
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't there be an unwind region here?

...
}
```
}];

let arguments = (ins UnitAttr:$synthetic, UnitAttr:$cleanup,
OptionalAttr<ArrayAttr>:$catch_types);
let regions = (region AnyRegion:$try_region,
VariadicRegion<AnyRegion>:$catch_regions);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see the unwind region (in cases where there is no catch-all) represented as a unique region rather than as a kind of catch region. It is specifically not a catch.


let assemblyFormat = [{
(`synthetic` $synthetic^)?
(`cleanup` $cleanup^)?
$try_region
custom<CatchRegions>($catch_regions, $catch_types)
attr-dict
}];

// Everything already covered elsewhere.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this comment mean?

let builders = [OpBuilder<(ins
"llvm::function_ref<void(mlir::OpBuilder &, "
"mlir::Location)>":$tryBuilder,
"llvm::function_ref<void(mlir::OpBuilder &, mlir::Location, "
"mlir::OperationState &)>":$catchBuilder)>];

let hasLLVMLowering = false;
}

//===----------------------------------------------------------------------===//
// Atomic operations
//===----------------------------------------------------------------------===//
Expand Down
132 changes: 132 additions & 0 deletions clang/lib/CIR/Dialect/IR/CIRDialect.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2878,6 +2878,138 @@ LogicalResult cir::TypeInfoAttr::verify(
return success();
}

//===----------------------------------------------------------------------===//
// TryOp
//===----------------------------------------------------------------------===//

void cir::TryOp::build(
OpBuilder &builder, OperationState &result,
function_ref<void(OpBuilder &, Location)> tryBuilder,
function_ref<void(OpBuilder &, Location, OperationState &)> catchBuilder) {
assert(tryBuilder && "expected builder callback for 'cir.try' body");
assert(catchBuilder && "expected builder callback for 'catch' body");

OpBuilder::InsertionGuard guard(builder);

// Try body region
Region *tryBodyRegion = result.addRegion();

// Create try body region and set insertion point
builder.createBlock(tryBodyRegion);
tryBuilder(builder, result.location);
catchBuilder(builder, result.location, result);
}

void cir::TryOp::getSuccessorRegions(
mlir::RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> &regions) {
// If any index all the underlying regions branch back to the parent
// operation.
if (!point.isParent()) {
regions.push_back(RegionSuccessor());
return;
}

// If the condition isn't constant, both regions may be executed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this comment belongs here. It looks like a copy+paste error.

regions.push_back(RegionSuccessor(&getTryRegion()));

// FIXME: optimize, ideas include:
// - If we know a target function never throws a specific type, we can
// remove the catch handler.
for (mlir::Region &r : this->getCatchRegions())
regions.push_back(RegionSuccessor(&r));
}

static void printCatchRegions(OpAsmPrinter &printer, cir::TryOp op,
mlir::MutableArrayRef<::mlir::Region> regions,
mlir::ArrayAttr catchersAttr) {
if (!catchersAttr)
return;

int currCatchIdx = 0;
printer << "catch [";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the square brackets around the catch region. Is there some precedent for that in other dialects?

Currently we have something that looks like this in the incubator:

      cir.try {
        cir.call exception @_Z3foov() : () -> ()
        cir.yield
      } catch [type #cir.global_view<@_ZTIPi> : !cir.ptr<!u8i> {
        %1 = cir.catch_param -> !cir.ptr<!s32i>
        cir.store align(8) %1, %0 : !cir.ptr<!s32i>, !cir.ptr<!cir.ptr<!s32i>>
        cir.yield
      }, type #cir.global_view<@_ZTIPf> : !cir.ptr<!u8i> {
        %2 = cir.catch_param -> !cir.ptr<!cir.float>
        cir.store align(8) %2, %0 : !cir.ptr<!cir.float>, !cir.ptr<!cir.ptr<!cir.float>>
        cir.yield
      }, #cir.unwind {
        cir.resume
      }]

I'd prefer to see something like this:

      cir.try {
        cir.call exception @_Z3foov() : () -> ()
        cir.yield
      } catch [type #cir.global_view<@_ZTIPi> : !cir.ptr<!u8i>] {
        %1 = cir.catch_param -> !cir.ptr<!s32i>
        cir.store align(8) %1, %0 : !cir.ptr<!s32i>, !cir.ptr<!cir.ptr<!s32i>>
        cir.yield
      } catch [type #cir.global_view<@_ZTIPf> : !cir.ptr<!u8i>] {
        %2 = cir.catch_param -> !cir.ptr<!cir.float>
        cir.store align(8) %2, %0 : !cir.ptr<!cir.float>, !cir.ptr<!cir.ptr<!cir.float>>
        cir.yield
      } unwind {
        cir.resume
      }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks much better, for a catch all, I think we don't need to print all as an attribute. Maybe we can print it like this? 🤔

Before

} catch [#cir.all] {
}

After

} catch all {
}

llvm::interleaveComma(catchersAttr, printer, [&](const Attribute &a) {
if (mlir::isa<cir::CatchUnwindAttr>(a)) {
printer.printAttribute(a);
printer << " ";
} else if (!a) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be handled by a CatchAllAttr?

printer << "all";
} else {
printer << "type ";
printer.printAttribute(a);
printer << " ";
}
printer.printRegion(regions[currCatchIdx], /*printEntryBLockArgs=*/false,
/*printBlockTerminators=*/true);
currCatchIdx++;
});

printer << "]";
}

static ParseResult parseCatchRegions(
OpAsmParser &parser,
llvm::SmallVectorImpl<std::unique_ptr<::mlir::Region>> &regions,
::mlir::ArrayAttr &catchersAttr) {
if (parser.parseKeyword("catch").failed())
return parser.emitError(parser.getCurrentLocation(),
"expected 'catch' keyword here");

auto parseAndCheckRegion = [&]() -> ParseResult {
// Parse region attached to catch
regions.emplace_back(new Region);
Region &currRegion = *regions.back();
SMLoc parserLoc = parser.getCurrentLocation();
if (parser.parseRegion(currRegion)) {
regions.clear();
return failure();
}

if (currRegion.empty()) {
return parser.emitError(parser.getCurrentLocation(),
"catch region shall not be empty");
}

if (!(currRegion.back().mightHaveTerminator() &&
currRegion.back().getTerminator()))
return parser.emitError(
parserLoc, "blocks are expected to be explicitly terminated");

return success();
};

llvm::SmallVector<mlir::Attribute, 4> catchList;
auto parseCatchEntry = [&]() -> ParseResult {
mlir::Attribute exceptionTypeInfo;

if (parser.parseOptionalAttribute(exceptionTypeInfo).has_value()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this parsing? It looks like it would handle any kind of attribute.

catchList.push_back(exceptionTypeInfo);
} else {
::llvm::StringRef attrStr;
if (parser.parseOptionalKeyword(&attrStr, {"all"}).succeeded()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems redundant.

// "all" keyword found, exceptionTypeInfo remains null
} else if (parser.parseOptionalKeyword("type").succeeded()) {
if (parser.parseAttribute(exceptionTypeInfo).failed())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should only accept a global view attribute referencing type info or the catch-all attribute (which I'd prefer to see handled differently). I think the code as written would accept any valid attribute.

return parser.emitError(parser.getCurrentLocation(),
"expected valid RTTI info attribute");
} else {
return parser.emitError(parser.getCurrentLocation(),
"expected attribute, 'all', or 'type' keyword");
}
catchList.push_back(exceptionTypeInfo);
}
return parseAndCheckRegion();
};

if (parser
.parseCommaSeparatedList(OpAsmParser::Delimiter::Square,
parseCatchEntry, " in catch list")
.failed())
return failure();

catchersAttr = parser.getBuilder().getArrayAttr(catchList);
return ::mlir::success();
}

//===----------------------------------------------------------------------===//
// TableGen'd op method definitions
//===----------------------------------------------------------------------===//
Expand Down
84 changes: 84 additions & 0 deletions clang/test/CIR/IR/try-catch.cir
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
// RUN: cir-opt %s --verify-roundtrip | FileCheck %s

!u8i = !cir.int<u, 8>

module {

cir.global "private" constant external @_ZTIi : !cir.ptr<!u8i>
cir.global "private" constant external @_ZTIPKc : !cir.ptr<!u8i>

cir.func dso_local @empty_try_block_with_catch_all() {
cir.scope {
cir.try {
cir.yield
} catch [type #cir.all {
cir.yield
}]
}
cir.return
}

// CHECK: cir.func dso_local @empty_try_block_with_catch_all() {
// CHECK: cir.scope {
// CHECK: cir.try {
// CHECK: cir.yield
// CHECK: } catch [type #cir.all {
// CHECK: cir.yield
// CHECK: }]
// CHECK: }
// CHECK: cir.return
// CHECK: }

cir.func dso_local @empty_try_block_with_catch_unwind() {
cir.scope {
cir.try {
cir.yield
} catch [#cir.unwind {
cir.yield
}]
}
cir.return
}

// CHECK: cir.func dso_local @empty_try_block_with_catch_unwind() {
// CHECK: cir.scope {
// CHECK: cir.try {
// CHECK: cir.yield
// CHECK: } catch [#cir.unwind {
// CHECK: cir.yield
// CHECK: }]
// CHECK: }
// CHECK: cir.return
// CHECK: }

cir.func dso_local @empty_try_block_with_catch_ist() {
cir.scope {
cir.try {
cir.yield
} catch [type #cir.global_view<@_ZTIi> : !cir.ptr<!u8i> {
cir.yield
}, type #cir.global_view<@_ZTIPKc> : !cir.ptr<!u8i> {
cir.yield
}, #cir.unwind {
cir.yield
}]
}
cir.return
}

// CHECK: cir.func dso_local @empty_try_block_with_catch_ist() {
// CHECK: cir.scope {
// CHECK: cir.try {
// CHECK: cir.yield
// CHECK: } catch [type #cir.global_view<@_ZTIi> : !cir.ptr<!u8i> {
// CHECK: cir.yield
// CHECK: }, type #cir.global_view<@_ZTIPKc> : !cir.ptr<!u8i> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a catch region?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because the current format is like a followed by an array of catcher's blocks, I agree it would be better to change the format to have catch before each block

// CHECK: cir.yield
// CHECK: }, #cir.unwind {
// CHECK: cir.yield
// CHECK: }]
// CHECK: }
// CHECK: cir.return
// CHECK: }

}