From 5d589f1962cfa0e944aa2f035d426acd2978b4a5 Mon Sep 17 00:00:00 2001 From: Christian Ulmann Date: Fri, 11 Jul 2025 11:59:53 +0000 Subject: [PATCH 1/2] [MLIR][Interfaces] Remove negative branch weight verifier This commit removes the verifier that checked if branch weights are negative. This check was too strict because weights are interpreted as unsigned integers. This showed up when running the verifier on LLVM dialect modules that were imported from LLVM IR. --- .../mlir/Interfaces/ControlFlowInterfaces.td | 28 +++++++++++-------- mlir/lib/Interfaces/ControlFlowInterfaces.cpp | 4 --- mlir/test/Dialect/ControlFlow/invalid.mlir | 12 -------- 3 files changed, 16 insertions(+), 28 deletions(-) diff --git a/mlir/include/mlir/Interfaces/ControlFlowInterfaces.td b/mlir/include/mlir/Interfaces/ControlFlowInterfaces.td index 46ab0b9ebbc6b..b8d08cc553caa 100644 --- a/mlir/include/mlir/Interfaces/ControlFlowInterfaces.td +++ b/mlir/include/mlir/Interfaces/ControlFlowInterfaces.td @@ -384,13 +384,15 @@ def WeightedBranchOpInterface : OpInterface<"WeightedBranchOpInterface"> { This interface provides weight information for branching terminator operations, i.e. terminator operations with successors. - This interface provides methods for getting/setting integer non-negative - weight of each branch. The probability of executing a branch - is computed as the ratio between the branch's weight and the total - sum of the weights (which cannot be zero). - The weights are optional. If they are provided, then their number + This interface provides methods for getting/setting integer weights of each + branch. The probability of executing a branch is computed as the ratio + between the branch's weight and the total sum of the weights (which cannot + be zero). The weights are optional. If they are provided, then their number must match the number of successors of the operation. + Note that the branch weight use an i32 representation but they are to be + interpreted as unsigned integers. + The default implementations of the methods expect the operation to have an attribute of type DenseI32ArrayAttr named branch_weights. }]; @@ -440,19 +442,21 @@ def WeightedRegionBranchOpInterface This interface provides weight information for region operations that exhibit branching behavior between held regions. - This interface provides methods for getting/setting integer non-negative - weight of each branch. The probability of executing a region is computed - as the ratio between the region branch's weight and the total sum - of the weights (which cannot be zero). - The weights are optional. If they are provided, then their number - must match the number of regions held by the operation - (including empty regions). + This interface provides methods for getting/setting integer weights of each + branch. The probability of executing a region is computed as the ratio + between the region branch's weight and the total sum of the weights (which + cannot be zero). The weights are optional. If they are provided, then their + number must match the number of regions held by the operation (including + empty regions). The weights specify the probability of branching to a particular region when first executing the operation. For example, for loop-like operations with a single region the weight specifies the probability of entering the loop. + Note that the branch weight use an i32 representation but they are to be + interpreted as unsigned integers. + The default implementations of the methods expect the operation to have an attribute of type DenseI32ArrayAttr named branch_weights. }]; diff --git a/mlir/lib/Interfaces/ControlFlowInterfaces.cpp b/mlir/lib/Interfaces/ControlFlowInterfaces.cpp index 3a63db35eec0f..e87bb461b0329 100644 --- a/mlir/lib/Interfaces/ControlFlowInterfaces.cpp +++ b/mlir/lib/Interfaces/ControlFlowInterfaces.cpp @@ -99,10 +99,6 @@ static LogicalResult verifyWeights(Operation *op, << ": " << weights.size() << " vs " << expectedWeightsNum; - for (auto [index, weight] : llvm::enumerate(weights)) - if (weight < 0) - return op->emitError() << "weight #" << index << " must be non-negative"; - if (llvm::all_of(weights, [](int32_t value) { return value == 0; })) return op->emitError() << "branch weights cannot all be zero"; diff --git a/mlir/test/Dialect/ControlFlow/invalid.mlir b/mlir/test/Dialect/ControlFlow/invalid.mlir index 1b8de22a9ff9f..0a71c62ec31af 100644 --- a/mlir/test/Dialect/ControlFlow/invalid.mlir +++ b/mlir/test/Dialect/ControlFlow/invalid.mlir @@ -82,18 +82,6 @@ func.func @wrong_weights_number(%cond: i1) { // ----- -// CHECK-LABEL: func @negative_weight -func.func @wrong_total_weight(%cond: i1) { - // expected-error@+1 {{weight #0 must be non-negative}} - cf.cond_br %cond weights([-1, 101]), ^bb1, ^bb2 - ^bb1: - return - ^bb2: - return -} - -// ----- - // CHECK-LABEL: func @zero_weights func.func @wrong_total_weight(%cond: i1) { // expected-error@+1 {{branch weights cannot all be zero}} From ffd648c266b1d50de894d7b3f284dfca14723820 Mon Sep 17 00:00:00 2001 From: Christian Ulmann Date: Fri, 11 Jul 2025 13:36:25 +0000 Subject: [PATCH 2/2] fix Fir test --- flang/test/Fir/invalid.fir | 9 --------- 1 file changed, 9 deletions(-) diff --git a/flang/test/Fir/invalid.fir b/flang/test/Fir/invalid.fir index e32ea7ad3c729..e5dbec44220b7 100644 --- a/flang/test/Fir/invalid.fir +++ b/flang/test/Fir/invalid.fir @@ -1426,12 +1426,3 @@ func.func @wrong_weights_number_in_if_then_else(%cond: i1) { } return } - -// ----- - -func.func @negative_weight_in_if_then(%cond: i1) { -// expected-error @below {{weight #0 must be non-negative}} - fir.if %cond weights([-1, 101]) { - } - return -}