From e169bf5ed345b357171794c5fd96c67bbb29511e Mon Sep 17 00:00:00 2001 From: Fangyi Zhou Date: Fri, 23 May 2025 12:33:38 +0100 Subject: [PATCH 1/5] [CIR] Add back verification of LoopOpInterface The verification used to require a runtime type query of `ConditionOp` which is not available outside the interface package; whereas the dialect package depends on the interface package, causing a circular dependency. This commit adds a singleton interface for `ConditionOp` to break off the circular dependency, and replaces the runtime type query using the newly added interface. Tested locally with a build with shared library enabled and verified that the build succeeds. --- clang/include/clang/CIR/Dialect/IR/CIROps.td | 9 ++++----- clang/include/clang/CIR/Interfaces/CIRLoopOpInterface.td | 7 +++++++ clang/lib/CIR/Interfaces/CIRLoopOpInterface.cpp | 9 ++++----- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/clang/include/clang/CIR/Dialect/IR/CIROps.td b/clang/include/clang/CIR/Dialect/IR/CIROps.td index ed66e83e0cd3..ae3cf90bd8ea 100644 --- a/clang/include/clang/CIR/Dialect/IR/CIROps.td +++ b/clang/include/clang/CIR/Dialect/IR/CIROps.td @@ -872,11 +872,10 @@ def SelectOp : CIR_Op<"select", [Pure, // ConditionOp //===----------------------------------------------------------------------===// -def ConditionOp : CIR_Op<"condition", [ - Terminator, - DeclareOpInterfaceMethods -]> { +def ConditionOp : CIR_Op<"condition", [Terminator, ConditionOpInterface, + DeclareOpInterfaceMethods< + RegionBranchTerminatorOpInterface, + ["getSuccessorRegions"]>]> { let summary = "Loop continuation condition."; let description = [{ The `cir.condition` terminates conditional regions. It takes a single diff --git a/clang/include/clang/CIR/Interfaces/CIRLoopOpInterface.td b/clang/include/clang/CIR/Interfaces/CIRLoopOpInterface.td index 8fd4a321b396..eee983f6f6a2 100644 --- a/clang/include/clang/CIR/Interfaces/CIRLoopOpInterface.td +++ b/clang/include/clang/CIR/Interfaces/CIRLoopOpInterface.td @@ -96,4 +96,11 @@ def LoopOpInterface : OpInterface<"LoopOpInterface", [ }]; } +def ConditionOpInterface : OpInterface<"ConditionOpInterface", []> { + let description = [{ + A singleton interface for ConditionOp only. This is used to break off a + a dependency from LoopOpInterface onto ConditionOp. + }]; + let cppNamespace = "::cir"; +} #endif // CLANG_CIR_INTERFACES_CIRLOOPOPINTERFACE diff --git a/clang/lib/CIR/Interfaces/CIRLoopOpInterface.cpp b/clang/lib/CIR/Interfaces/CIRLoopOpInterface.cpp index 085e1f6c245d..e3f508bcdf1c 100644 --- a/clang/lib/CIR/Interfaces/CIRLoopOpInterface.cpp +++ b/clang/lib/CIR/Interfaces/CIRLoopOpInterface.cpp @@ -44,11 +44,10 @@ void LoopOpInterface::getLoopOpSuccessorRegions( /// Verify invariants of the LoopOpInterface. llvm::LogicalResult detail::verifyLoopOpInterface(mlir::Operation *op) { - // FIXME: fix this so the conditionop isn't requiring MLIRCIR - // auto loopOp = mlir::cast(op); - // if (!mlir::isa(loopOp.getCond().back().getTerminator())) - // return op->emitOpError( - // "expected condition region to terminate with 'cir.condition'"); + auto loopOp = mlir::cast(op); + if (!mlir::isa(loopOp.getCond().back().getTerminator())) + return op->emitOpError( + "expected condition region to terminate with 'cir.condition'"); return llvm::success(); } From b165b014d9a1d18eb81da403260009d6d9d414bf Mon Sep 17 00:00:00 2001 From: Fangyi Zhou Date: Thu, 22 May 2025 00:05:42 +0100 Subject: [PATCH 2/5] Unxfail tests --- clang/test/CIR/IR/invalid.cir | 41 ++++++++++++++++++++++++++++ clang/test/CIR/IR/invalid_xfail.cir | 42 ----------------------------- 2 files changed, 41 insertions(+), 42 deletions(-) delete mode 100644 clang/test/CIR/IR/invalid_xfail.cir diff --git a/clang/test/CIR/IR/invalid.cir b/clang/test/CIR/IR/invalid.cir index 30fc93ecb979..3e8f831afdae 100644 --- a/clang/test/CIR/IR/invalid.cir +++ b/clang/test/CIR/IR/invalid.cir @@ -1533,3 +1533,44 @@ cir.global external dsolocal @vfp = #cir.ptr : !cir.ptr> + +// ----- + +#false = #cir.bool : !cir.bool +#true = #cir.bool : !cir.bool +cir.func @b0() { + cir.scope { + cir.while { // expected-error {{expected condition region to terminate with 'cir.condition'}} + cir.yield + } do { + cir.br ^bb1 + ^bb1: + cir.return + } + } + cir.return +} + +// ----- + +cir.func @invalid_cond_region_terminator(%arg0 : !cir.bool) -> !cir.void { + cir.do { // expected-error {{op expected condition region to terminate with 'cir.condition'}} + cir.yield + } while { + cir.yield + } + cir.return +} + +// ----- + +cir.func @invalidConditionTerminator (%arg0 : !cir.bool) -> !cir.void { + cir.for : cond { // expected-error {{op expected condition region to terminate with 'cir.condition'}} + cir.yield + } body { + cir.yield + } step { + cir.yield + } + cir.return +} diff --git a/clang/test/CIR/IR/invalid_xfail.cir b/clang/test/CIR/IR/invalid_xfail.cir deleted file mode 100644 index c29dbf075b6b..000000000000 --- a/clang/test/CIR/IR/invalid_xfail.cir +++ /dev/null @@ -1,42 +0,0 @@ -// Test attempts to build bogus CIR -// RUN: cir-opt %s -verify-diagnostics -split-input-file -// XFAIL: * - -#false = #cir.bool : !cir.bool -#true = #cir.bool : !cir.bool -cir.func @b0() { - cir.scope { - cir.while { // expected-error {{expected condition region to terminate with 'cir.condition'}} - cir.yield - } do { - cir.br ^bb1 - ^bb1: - cir.return - } - } - cir.return -} - -// ----- - -cir.func @invalid_cond_region_terminator(%arg0 : !cir.bool) -> !cir.void { - cir.do { // expected-error {{op expected condition region to terminate with 'cir.condition'}} - cir.yield - } while { - cir.yield - } - cir.return -} - -// ----- - -cir.func @invalidConditionTerminator (%arg0 : !cir.bool) -> !cir.void { - cir.for : cond { // expected-error {{op expected condition region to terminate with 'cir.condition'}} - cir.yield - } body { - cir.yield - } step { - cir.yield - } - cir.return -} From a98341f1377b87bc138af1c11c84e47ceb826ac3 Mon Sep 17 00:00:00 2001 From: Fangyi Zhou Date: Fri, 23 May 2025 22:44:44 +0100 Subject: [PATCH 3/5] Minor fixes --- clang/include/clang/CIR/Interfaces/CIRLoopOpInterface.td | 3 ++- clang/lib/CIR/Interfaces/CIRLoopOpInterface.cpp | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/clang/include/clang/CIR/Interfaces/CIRLoopOpInterface.td b/clang/include/clang/CIR/Interfaces/CIRLoopOpInterface.td index eee983f6f6a2..56e1c86d1b53 100644 --- a/clang/include/clang/CIR/Interfaces/CIRLoopOpInterface.td +++ b/clang/include/clang/CIR/Interfaces/CIRLoopOpInterface.td @@ -96,11 +96,12 @@ def LoopOpInterface : OpInterface<"LoopOpInterface", [ }]; } -def ConditionOpInterface : OpInterface<"ConditionOpInterface", []> { +def ConditionOpInterface : OpInterface<"CIR_ConditionOpInterface", []> { let description = [{ A singleton interface for ConditionOp only. This is used to break off a a dependency from LoopOpInterface onto ConditionOp. }]; let cppNamespace = "::cir"; } + #endif // CLANG_CIR_INTERFACES_CIRLOOPOPINTERFACE diff --git a/clang/lib/CIR/Interfaces/CIRLoopOpInterface.cpp b/clang/lib/CIR/Interfaces/CIRLoopOpInterface.cpp index e3f508bcdf1c..cfe9366859b8 100644 --- a/clang/lib/CIR/Interfaces/CIRLoopOpInterface.cpp +++ b/clang/lib/CIR/Interfaces/CIRLoopOpInterface.cpp @@ -45,7 +45,8 @@ void LoopOpInterface::getLoopOpSuccessorRegions( /// Verify invariants of the LoopOpInterface. llvm::LogicalResult detail::verifyLoopOpInterface(mlir::Operation *op) { auto loopOp = mlir::cast(op); - if (!mlir::isa(loopOp.getCond().back().getTerminator())) + if (!mlir::isa( + loopOp.getCond().back().getTerminator())) return op->emitOpError( "expected condition region to terminate with 'cir.condition'"); return llvm::success(); From bd5292a4f92c560dc938c8fc065b74b165e50aa3 Mon Sep 17 00:00:00 2001 From: Fangyi Zhou Date: Sat, 24 May 2025 16:10:51 +0100 Subject: [PATCH 4/5] Prefix CIR to tblgen identifier --- clang/include/clang/CIR/Dialect/IR/CIROps.td | 2 +- clang/include/clang/CIR/Interfaces/CIRLoopOpInterface.td | 2 +- clang/lib/CIR/Interfaces/CIRLoopOpInterface.cpp | 3 +-- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/clang/include/clang/CIR/Dialect/IR/CIROps.td b/clang/include/clang/CIR/Dialect/IR/CIROps.td index ae3cf90bd8ea..73ff2b5aec97 100644 --- a/clang/include/clang/CIR/Dialect/IR/CIROps.td +++ b/clang/include/clang/CIR/Dialect/IR/CIROps.td @@ -872,7 +872,7 @@ def SelectOp : CIR_Op<"select", [Pure, // ConditionOp //===----------------------------------------------------------------------===// -def ConditionOp : CIR_Op<"condition", [Terminator, ConditionOpInterface, +def ConditionOp : CIR_Op<"condition", [Terminator, CIR_ConditionOpInterface, DeclareOpInterfaceMethods< RegionBranchTerminatorOpInterface, ["getSuccessorRegions"]>]> { diff --git a/clang/include/clang/CIR/Interfaces/CIRLoopOpInterface.td b/clang/include/clang/CIR/Interfaces/CIRLoopOpInterface.td index 56e1c86d1b53..704a161a2c24 100644 --- a/clang/include/clang/CIR/Interfaces/CIRLoopOpInterface.td +++ b/clang/include/clang/CIR/Interfaces/CIRLoopOpInterface.td @@ -96,7 +96,7 @@ def LoopOpInterface : OpInterface<"LoopOpInterface", [ }]; } -def ConditionOpInterface : OpInterface<"CIR_ConditionOpInterface", []> { +def CIR_ConditionOpInterface : OpInterface<"ConditionOpInterface", []> { let description = [{ A singleton interface for ConditionOp only. This is used to break off a a dependency from LoopOpInterface onto ConditionOp. diff --git a/clang/lib/CIR/Interfaces/CIRLoopOpInterface.cpp b/clang/lib/CIR/Interfaces/CIRLoopOpInterface.cpp index cfe9366859b8..e3f508bcdf1c 100644 --- a/clang/lib/CIR/Interfaces/CIRLoopOpInterface.cpp +++ b/clang/lib/CIR/Interfaces/CIRLoopOpInterface.cpp @@ -45,8 +45,7 @@ void LoopOpInterface::getLoopOpSuccessorRegions( /// Verify invariants of the LoopOpInterface. llvm::LogicalResult detail::verifyLoopOpInterface(mlir::Operation *op) { auto loopOp = mlir::cast(op); - if (!mlir::isa( - loopOp.getCond().back().getTerminator())) + if (!mlir::isa(loopOp.getCond().back().getTerminator())) return op->emitOpError( "expected condition region to terminate with 'cir.condition'"); return llvm::success(); From 3da67258b8e07209ca7bb55a36b640d95c6209e7 Mon Sep 17 00:00:00 2001 From: Fangyi Zhou Date: Sat, 24 May 2025 16:14:45 +0100 Subject: [PATCH 5/5] Prefix CIR to tblgen identifier for LoopOpInterface --- clang/include/clang/CIR/Dialect/IR/CIROps.td | 8 +++----- clang/include/clang/CIR/Interfaces/CIRLoopOpInterface.td | 8 ++++---- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/clang/include/clang/CIR/Dialect/IR/CIROps.td b/clang/include/clang/CIR/Dialect/IR/CIROps.td index 73ff2b5aec97..5f491a2372c2 100644 --- a/clang/include/clang/CIR/Dialect/IR/CIROps.td +++ b/clang/include/clang/CIR/Dialect/IR/CIROps.td @@ -2194,10 +2194,8 @@ def BrCondOp : CIR_Op<"brcond", // While & DoWhileOp //===----------------------------------------------------------------------===// -class WhileOpBase : CIR_Op { +class WhileOpBase + : CIR_Op { defvar isWhile = !eq(mnemonic, "while"); let summary = "C/C++ " # !if(isWhile, "while", "do-while") # " loop"; let builders = [ @@ -2272,7 +2270,7 @@ def DoWhileOp : WhileOpBase<"do"> { // ForOp //===----------------------------------------------------------------------===// -def ForOp : CIR_Op<"for", [LoopOpInterface, NoRegionArguments]> { +def ForOp : CIR_Op<"for", [CIR_LoopOpInterface, NoRegionArguments]> { let summary = "C/C++ for loop counterpart"; let description = [{ Represents a C/C++ for loop. It consists of three regions: diff --git a/clang/include/clang/CIR/Interfaces/CIRLoopOpInterface.td b/clang/include/clang/CIR/Interfaces/CIRLoopOpInterface.td index 704a161a2c24..36b71707ee65 100644 --- a/clang/include/clang/CIR/Interfaces/CIRLoopOpInterface.td +++ b/clang/include/clang/CIR/Interfaces/CIRLoopOpInterface.td @@ -13,10 +13,10 @@ include "mlir/IR/OpBase.td" include "mlir/Interfaces/ControlFlowInterfaces.td" include "mlir/Interfaces/LoopLikeInterface.td" -def LoopOpInterface : OpInterface<"LoopOpInterface", [ - DeclareOpInterfaceMethods, - DeclareOpInterfaceMethods -]> { +def CIR_LoopOpInterface + : OpInterface<"LoopOpInterface", + [DeclareOpInterfaceMethods, + DeclareOpInterfaceMethods]> { let description = [{ Contains helper functions to query properties and perform transformations on a loop.