This is an archive of the discontinued LLVM Phabricator instance.

Differential D108402

[MLIR][SCF] Parenthesize multiple return types in scf.execute_region asm op
ClosedPublic

Authored by mortbopet on Aug 19 2021, 12:39 PM.

Details

Reviewers
rriddle
Commits
rG6c1436a9b03e: [MLIR][SCF] Parenthesize multiple return types in scf.execute_region asm op
Summary

Previously, ExecuteRegionOps with multiple return values would fail a round-trip test due to missing parenthesis around the types.

Diff Detail

Event Timeline

mortbopet created this revision.Aug 19 2021, 12:39 PM
Herald added subscribers: wrengr, Chia-hungDuan, dcaballe and 18 others. · View Herald TranscriptAug 19 2021, 12:39 PM
mortbopet requested review of this revision.Aug 19 2021, 12:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2021, 12:39 PM
Herald added subscribers: stephenneuendorffer, nicolasvasilache. · View Herald Transcript
rriddle added inline comments.Aug 19 2021, 12:46 PM
mlir/lib/Dialect/SCF/SCF.cpp
118–121

^ That should do the same.

mortbopet updated this revision to Diff 367594.Aug 19 2021, 12:52 PM

[MLIR][SCF] Use printOptionalArrowTypeList

rriddle accepted this revision.Aug 19 2021, 1:10 PM
rriddle added inline comments.
mlir/test/Dialect/SCF/ops.mlir
295–297

Are these lines necessary for the test? I'd drop them. This test should just be about the trailing types.

This revision is now accepted and ready to land.Aug 19 2021, 1:10 PM
mortbopet updated this revision to Diff 367600.Aug 19 2021, 1:18 PM
mortbopet marked an inline comment as done.

Remove redundant check lines

This revision was landed with ongoing or failed builds.Aug 19 2021, 1:33 PM
Closed by commit rG6c1436a9b03e: [MLIR][SCF] Parenthesize multiple return types in scf.execute_region asm op (authored by mortbopet). · Explain Why
This revision was automatically updated to reflect the committed changes.
mortbopet added a commit: rG6c1436a9b03e: [MLIR][SCF] Parenthesize multiple return types in scf.execute_region asm op.
Harbormaster completed remote builds in B120412: Diff 367600.Aug 19 2021, 1:52 PM

Revision Contents

PathSize
mlir/
lib/
Dialect/
SCF/
3 lines
test/
Dialect/
SCF/
6 lines

Diff 367604

mlir/lib/Dialect/SCF/SCF.cpp

Show First 20 Lines • Show All 109 Lines • ▼ Show 20 Lines if (parser.parseRegion(*body, /*arguments=*/{}, /*argTypes=*/{}) ||
parser.parseOptionalAttrDict(result.attributes)) parser.parseOptionalAttrDict(result.attributes))
return failure(); return failure();
return success(); return success();
} }
static void print(OpAsmPrinter &p, ExecuteRegionOp op) { static void print(OpAsmPrinter &p, ExecuteRegionOp op) {
p << ExecuteRegionOp::getOperationName(); p << ExecuteRegionOp::getOperationName();
if (op.getNumResults() > 0) p.printOptionalArrowTypeList(op.getResultTypes());
p << " -> " << op.getResultTypes();
p.printRegion(op.region(), p.printRegion(op.region(),
/*printEntryBlockArgs=*/false, /*printEntryBlockArgs=*/false,
rriddleUnsubmitted
Done
ReplyInline Actions
p << ExecuteRegionOp::getOperationName();
- bool withParens = op.getResultTypes().size() > 1;
- if (op.getNumResults() > 0)
- p << " -> " << (withParens ? "(" : "") << op.getResultTypes()
- << (withParens ? ")" : "");
+ p.printOptionalArrowTypeList(op.getResultTypes());
p.printRegion(op.region(),

^ That should do the same.

rriddle: ^ That should do the same.
/*printBlockTerminators=*/true); /*printBlockTerminators=*/true);
p.printOptionalAttrDict(op->getAttrs()); p.printOptionalAttrDict(op->getAttrs());
} }
static LogicalResult verify(ExecuteRegionOp op) { static LogicalResult verify(ExecuteRegionOp op) {
if (op.region().empty()) if (op.region().empty())
return op.emitOpError("region needs to have at least one block"); return op.emitOpError("region needs to have at least one block");
▲ Show 20 Lines • Show All 2,123 Lines • Show Last 20 Lines

mlir/test/Dialect/SCF/ops.mlir

Show First 20 Lines • Show All 285 Lines • ▼ Show 20 Linesfunc @execute_region() -> i64 {
// CHECK-NEXT: constant // CHECK-NEXT: constant
// CHECK-NEXT: scf.yield // CHECK-NEXT: scf.yield
// CHECK-NEXT: } // CHECK-NEXT: }
%res = scf.execute_region -> i64 { %res = scf.execute_region -> i64 {
%c1 = constant 1 : i64 %c1 = constant 1 : i64
scf.yield %c1 : i64 scf.yield %c1 : i64
} }
// CHECK: scf.execute_region -> (i64, i64) {
%res2:2 = scf.execute_region -> (i64, i64) {
%c1 = constant 1 : i64
scf.yield %c1, %c1 : i64, i64
rriddleUnsubmitted
Not Done
ReplyInline Actions

Are these lines necessary for the test? I'd drop them. This test should just be about the trailing types.

rriddle: Are these lines necessary for the test? I'd drop them. This test should just be about the…
}
// CHECK: scf.execute_region { // CHECK: scf.execute_region {
// CHECK-NEXT: br ^bb1 // CHECK-NEXT: br ^bb1
// CHECK-NEXT: ^bb1: // CHECK-NEXT: ^bb1:
// CHECK-NEXT: scf.yield // CHECK-NEXT: scf.yield
// CHECK-NEXT: } // CHECK-NEXT: }
"scf.execute_region"() ({ "scf.execute_region"() ({
^bb0: ^bb0:
br ^bb1 br ^bb1
^bb1: ^bb1:
scf.yield scf.yield
}) : () -> () }) : () -> ()
return %res : i64 return %res : i64
} }