-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[CIR] Upstream CIR Dialect TryOp with Catch Attrs #162897
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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", [ | ||||||||||||
|
@@ -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", [ | ||||||||||||
|
@@ -4296,6 +4296,81 @@ 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`). | ||||||||||||
|
||||||||||||
`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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||||||||||||
|
||||||||||||
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 | ||||||||||||
} | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes sense, the problem is that we can't add it after the Variadic regions. If we added it before, it works, but when TableGen generate I was thinking of changing the name of Not sure if there is a workaround in tablegen @xlauko 🤔 |
||||||||||||
|
||||||||||||
let assemblyFormat = [{ | ||||||||||||
(`synthetic` $synthetic^)? | ||||||||||||
(`cleanup` $cleanup^)? | ||||||||||||
$try_region | ||||||||||||
custom<CatchRegions>($catch_regions, $catch_types) | ||||||||||||
attr-dict | ||||||||||||
}]; | ||||||||||||
|
||||||||||||
// Everything already covered elsewhere. | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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), | ||||||||||||
[{ | ||||||||||||
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 = $_state.addRegion(); | ||||||||||||
|
||||||||||||
// Create try body region and set insertion point | ||||||||||||
$_builder.createBlock(tryBodyRegion); | ||||||||||||
tryBuilder($_builder, $_state.location); | ||||||||||||
catchBuilder($_builder, $_state.location, $_state); | ||||||||||||
}]> | ||||||||||||
]; | ||||||||||||
|
||||||||||||
let hasLLVMLowering = false; | ||||||||||||
} | ||||||||||||
|
||||||||||||
//===----------------------------------------------------------------------===// | ||||||||||||
// Atomic operations | ||||||||||||
//===----------------------------------------------------------------------===// | ||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2878,6 +2878,120 @@ LogicalResult cir::TypeInfoAttr::verify( | |
return success(); | ||
} | ||
|
||
//===----------------------------------------------------------------------===// | ||
// TryOp | ||
//===----------------------------------------------------------------------===// | ||
|
||
void cir::TryOp::getSuccessorRegions( | ||
mlir::RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> ®ions) { | ||
// 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ["; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
I'd prefer to see something like this:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Before
After
|
||
llvm::interleaveComma(catchersAttr, printer, [&](const Attribute &a) { | ||
if (mlir::isa<cir::CatchUnwindAttr>(a)) { | ||
printer.printAttribute(a); | ||
printer << " "; | ||
} else if (!a) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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>> ®ions, | ||
::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"); | ||
AmrDeveloper marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
return success(); | ||
}; | ||
|
||
llvm::SmallVector<mlir::Attribute, 4> catchList; | ||
auto parseCatchEntry = [&]() -> ParseResult { | ||
mlir::Attribute exceptionTypeInfo; | ||
|
||
if (parser.parseOptionalAttribute(exceptionTypeInfo).has_value()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(), | ||
AmrDeveloper marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"expected valid RTTI info attribute"); | ||
} else { | ||
return parser.emitError(parser.getCurrentLocation(), | ||
"expected attribute, 'all', or 'type' keyword"); | ||
AmrDeveloper marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
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 | ||
//===----------------------------------------------------------------------===// | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
// RUN: cir-opt %s -verify-diagnostics -split-input-file | ||
|
||
module { | ||
|
||
cir.func dso_local @invalid_catch_without_all_or_type() { | ||
cir.scope { | ||
cir.try { | ||
cir.yield | ||
// expected-error @below {{'cir.try' expected attribute, 'all', or 'type' keyword}} | ||
} catch [invalid_keyword { | ||
cir.yield | ||
}] | ||
} | ||
cir.return | ||
} | ||
|
||
} | ||
|
||
// ----- | ||
|
||
module { | ||
|
||
cir.func dso_local @invalid_catch_rtti_type() { | ||
cir.scope { | ||
cir.try { | ||
cir.yield | ||
// expected-error @below {{expected attribute value}} | ||
// expected-error @below {{'cir.try' expected valid RTTI info attribute}} | ||
} catch [type invalid_type { | ||
cir.yield | ||
}] | ||
} | ||
cir.return | ||
} | ||
|
||
} | ||
|
||
// ----- | ||
|
||
module { | ||
|
||
cir.func dso_local @invalid_catch_empty_block() { | ||
cir.scope { | ||
cir.try { | ||
cir.yield | ||
} catch [type #cir.all { | ||
// expected-error @below {{'cir.try' catch region shall not be empty}} | ||
}] | ||
} | ||
cir.return | ||
} | ||
|
||
} | ||
|
||
// ----- | ||
|
||
!s32i = !cir.int<s, 32> | ||
|
||
module { | ||
|
||
cir.func dso_local @invalid_catch_not_terminated() { | ||
%a = cir.alloca !s32i, !cir.ptr<!s32i>, ["a", init] | ||
cir.scope { | ||
cir.try { | ||
cir.yield | ||
} | ||
// expected-error @below {{'cir.try' blocks are expected to be explicitly terminated}} | ||
catch [type #cir.all { | ||
%tmp_a = cir.load %a : !cir.ptr<!s32i>, !s32i | ||
}] | ||
} | ||
cir.return | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.