From 16400200f20037868189e892e96a4bf573d3195b Mon Sep 17 00:00:00 2001 From: Roberto Bertolini Date: Fri, 16 May 2025 11:48:15 +0200 Subject: [PATCH 1/2] Implement beautifier pass for Clift switch cases --- .../Clift/Transforms/Beautify/Statements.cpp | 54 +++++++++ .../switch-case-rewrite.mlir | 107 ++++++++++++++++++ 2 files changed, 161 insertions(+) create mode 100644 tests/unit/mlir_lit_tests/statement-rewrites/switch-case-rewrite.mlir diff --git a/lib/mlir/Dialect/Clift/Transforms/Beautify/Statements.cpp b/lib/mlir/Dialect/Clift/Transforms/Beautify/Statements.cpp index 9756e34a7b..77c01d5825 100644 --- a/lib/mlir/Dialect/Clift/Transforms/Beautify/Statements.cpp +++ b/lib/mlir/Dialect/Clift/Transforms/Beautify/Statements.cpp @@ -692,6 +692,59 @@ struct OptimizedWhileConversionPattern : mlir::RewritePattern { }; #endif + +struct SwitchCaseRewritePattern : mlir::OpRewritePattern { + using OpRewritePattern::OpRewritePattern; + + void initialize() { setDebugName("switch-case-rewrite"); } + + mlir::LogicalResult + matchAndRewrite(clift::SwitchOp SwitchOp, + mlir::PatternRewriter &Rewriter) const override { + // Reject if there is more than one case + if (SwitchOp.getNumCases() != 1) { + return Rewriter.notifyMatchFailure(SwitchOp, + "Only single-case SwitchOps can be " + "rewritten"); + } + + Rewriter.setInsertionPoint(SwitchOp); + + // Create the IfOp at the position of the switchOp + clift::IfOp IfOp = Rewriter.create(SwitchOp.getLoc()); + + // The ifOp inherits the regions of the switchOp + IfOp.getCondition().takeBody(SwitchOp.getCondition()); + IfOp.getThen().takeBody(SwitchOp.getCaseRegion(0)); + IfOp.getElse().takeBody(SwitchOp.getDefaultCaseRegion()); + + uint64_t CaseValue = SwitchOp.getCaseValue(0); + + // Erase the switchOp + Rewriter.eraseOp(SwitchOp); + + replaceExpression(Rewriter, IfOp.getCondition(), [&](mlir::Value Value) { + // Create the immediate op for the case value + auto CaseValueOp = Rewriter.create(IfOp.getLoc(), + Value.getType(), + CaseValue); + + // Create a comparison op + auto BoolType = getBooleanType(Rewriter.getContext()); + auto CmpOp = Rewriter.create(IfOp.getLoc(), + BoolType, + Value, + CaseValueOp.getResult()); + + // Yield the result of the comparison + return CmpOp.getResult(); + }); + + return mlir::success(); + } +}; + + struct BeautifyStatementsPass : clift::impl::CliftBeautifyStatementsBase { mlir::LogicalResult initialize(mlir::MLIRContext *Context) override { @@ -707,6 +760,7 @@ struct BeautifyStatementsPass Set.add(Context); Set.add(Context); Set.add(Context); + Set.add(Context); Patterns = mlir::FrozenRewritePatternSet(std::move(Set), disabledPatterns, diff --git a/tests/unit/mlir_lit_tests/statement-rewrites/switch-case-rewrite.mlir b/tests/unit/mlir_lit_tests/statement-rewrites/switch-case-rewrite.mlir new file mode 100644 index 0000000000..ec4d8e4f2a --- /dev/null +++ b/tests/unit/mlir_lit_tests/statement-rewrites/switch-case-rewrite.mlir @@ -0,0 +1,107 @@ +// +// This file is distributed under the MIT License. See LICENSE.md for details. +// + +// RUN: %revngcliftopt %s --beautify-statements="enable-patterns=switch-case-rewrite" | FileCheck %s + +!void = !clift.primitive +!int32_t = !clift.primitive + +!f = !clift.func< + "/type-definition/1001-CABIFunctionDefinition" : !void() +> + +module attributes {clift.module} { + clift.func @f() attributes { + } { + // CHECK: [[L:%[0-9]+]] = clift.local !int32_t + %0 = clift.local !int32_t + + // CHECK: clift.if { + clift.switch { + // CHECK: [[R1:%[0-9]+]] = clift.imm 1 : !int32_t + // CHECK: [[R2:%[0-9]+]] = clift.eq [[L]], [[R1]] : !int32_t -> !int8_t + clift.yield %0: !int32_t + // CHECK: clift.yield [[R2]] : !int8_t + // CHECK: } { + } case 1 { + // CHECK: clift.expr { + clift.expr { + // CHECK: [[R3:%[0-9]+]] = clift.imm 1 : !int32_t + %1 = clift.imm 1 : !int32_t + // CHECK: clift.yield [[R3]] : !int32_t + clift.yield %1 : !int32_t + // CHECK: } + } + // CHECK: } else { + } default { + // CHECK: clift.expr { + clift.expr { + // CHECK: [[R4:%[0-9]+]] = clift.imm 2 : !int32_t + %1 = clift.imm 2 : !int32_t + // CHECK: clift.yield [[R4]] : !int32_t + clift.yield %1 : !int32_t + // CHECK: } + } + // CHECK: } + } + + // COM: Different condition region + // CHECK: clift.if { + clift.switch { + // CHECK: [[L2:%[0-9]+]] = clift.imm 1 : !int32_t + %1 = clift.imm 1 : !int32_t + // CHECK: [[R5:%[0-9]+]] = clift.imm 3 : !int32_t + // CHECK: [[R6:%[0-9]+]] = clift.eq [[L2]], [[R5]] : !int32_t -> !int8_t + clift.yield %1: !int32_t + // CHECK: clift.yield [[R6]] : !int8_t + // CHECK: } { + } case 3 { + // CHECK: clift.expr { + clift.expr { + // CHECK: [[R7:%[0-9]+]] = clift.imm 1 : !int32_t + %1 = clift.imm 1 : !int32_t + // CHECK: clift.yield [[R7]] : !int32_t + clift.yield %1 : !int32_t + // CHECK: } + } + // CHECK: } else { + } default { + // CHECK: clift.expr { + clift.expr { + // CHECK: [[R8:%[0-9]+]] = clift.imm 2 : !int32_t + %1 = clift.imm 2 : !int32_t + // CHECK: clift.yield [[R8]] : !int32_t + clift.yield %1 : !int32_t + // CHECK: } + } + // CHECK: } + } + + // COM: This shouldn't get rewritten + // CHECK: clift.switch { + clift.switch { + clift.yield %0: !int32_t + // CHECK: } case 1 { + } case 1 { + clift.expr { + %1 = clift.imm 1 : !int32_t + clift.yield %1 : !int32_t + } + // CHECK: } case 2 { + } case 2 { + clift.expr { + %1 = clift.imm 1 : !int32_t + clift.yield %1 : !int32_t + } + // CHECK: } default { + } default { + clift.expr { + %1 = clift.imm 2 : !int32_t + clift.yield %1 : !int32_t + } + // CHECK: } + } + } + // CHECK: } +} From 5e69d1598bd6102444bc86a6971f27943dfbd7f3 Mon Sep 17 00:00:00 2001 From: Roberto Bertolini Date: Thu, 22 May 2025 12:41:38 +0200 Subject: [PATCH 2/2] Implement scope tightening for locals in functions --- .../mlir/Dialect/Clift/Transforms/Passes.h | 2 + .../mlir/Dialect/Clift/Transforms/Passes.td | 5 + .../Beautify/TightenVariableScopes.cpp | 165 +++++++++++++++ .../Dialect/Clift/Transforms/CMakeLists.txt | 1 + .../statement-rewrites/scope-tightening.mlir | 195 ++++++++++++++++++ 5 files changed, 368 insertions(+) create mode 100644 lib/mlir/Dialect/Clift/Transforms/Beautify/TightenVariableScopes.cpp create mode 100644 tests/unit/mlir_lit_tests/statement-rewrites/scope-tightening.mlir diff --git a/include/revng/mlir/Dialect/Clift/Transforms/Passes.h b/include/revng/mlir/Dialect/Clift/Transforms/Passes.h index 139d49fca1..5f49182962 100644 --- a/include/revng/mlir/Dialect/Clift/Transforms/Passes.h +++ b/include/revng/mlir/Dialect/Clift/Transforms/Passes.h @@ -28,6 +28,8 @@ PassPtr createImmediateRadixDeductionPass(); PassPtr createVerifyCPass(); +PassPtr createTightenVariableScopePass(); + #define GEN_PASS_REGISTRATION #include "revng/mlir/Dialect/Clift/Transforms/Passes.h.inc" diff --git a/include/revng/mlir/Dialect/Clift/Transforms/Passes.td b/include/revng/mlir/Dialect/Clift/Transforms/Passes.td index 433605b366..a72c83b05a 100644 --- a/include/revng/mlir/Dialect/Clift/Transforms/Passes.td +++ b/include/revng/mlir/Dialect/Clift/Transforms/Passes.td @@ -61,6 +61,11 @@ def CliftImmediateRadixDeduction : Clift_Pass<"deduce-immediate-radices", let constructor = "mlir::clift::createImmediateRadixDeductionPass()"; } +def CliftTightenVariableScopes : Clift_Pass<"tighten-variable-scopes", "clift::FunctionOp"> { + let summary = "Tighten variable scopes"; + let constructor = "mlir::clift::createTightenVariableScopePass()"; +} + def CliftVerifyC : Clift_Pass<"verify-c", "mlir::ModuleOp"> { let summary = "Verify that the Clift semantics correspond to C semantics " # "in the specified target implementation."; diff --git a/lib/mlir/Dialect/Clift/Transforms/Beautify/TightenVariableScopes.cpp b/lib/mlir/Dialect/Clift/Transforms/Beautify/TightenVariableScopes.cpp new file mode 100644 index 0000000000..5f309c491f --- /dev/null +++ b/lib/mlir/Dialect/Clift/Transforms/Beautify/TightenVariableScopes.cpp @@ -0,0 +1,165 @@ +// +// This file is distributed under the MIT License. See LICENSE.md for details. +// + +#include "llvm/ADT/SmallVector.h" + +#include "mlir/Pass/Pass.h" + +#include "revng/mlir/Dialect/Clift/IR/CliftOpHelpers.h" +#include "revng/mlir/Dialect/Clift/Transforms/Passes.h" + +namespace mlir { +namespace clift { +#define GEN_PASS_DEF_CLIFTTIGHTENVARIABLESCOPES +#include "revng/mlir/Dialect/Clift/Transforms/Passes.h.inc" +} // namespace clift +} // namespace mlir + +namespace clift = mlir::clift; + +namespace { + +// Stores the position and nesting level for each local variable definition +struct LocalVariableLocation { + clift::BlockPosition Position; + unsigned NestingLevel = 0; +}; + +// Custom walk function that provides nesting level +template +static void walkWithNestingLevel(mlir::Region *Region, + CallbackT Callback, + unsigned NestingLevel = 0) { + for (mlir::Block &Block : *Region) { + for (mlir::Operation &Op : Block) { + // Post-order walk through the nested regions, incrementing the nesting + // level for inner region + for (mlir::Region &NestedRegion : Op.getRegions()) { + walkWithNestingLevel(&NestedRegion, Callback, NestingLevel + 1); + } + + // Then call the callback on the operation itself + Callback(&Op, NestingLevel); + } + } +} + +// Merges the current location for a variable definition with the new use +// position +static void updateVariableLocation(LocalVariableLocation &VarLoc, + clift::BlockPosition NewPosition, + unsigned NewLevel) { + mlir::Region *CurrentRegion = VarLoc.Position.Block->getParent(); + mlir::Region *NewPosRegion = NewPosition.Block->getParent(); + + unsigned CurrentLevel = VarLoc.NestingLevel; + + // If the new position is at a lower nesting level, we surely need to + // go up from the current region at least until the levels are equal, + // in order to find the common ancestor + while (CurrentLevel > NewLevel) { + CurrentRegion = CurrentRegion->getParentRegion(); + CurrentLevel--; + } + + // Conversely, if the new position is at a higher nesting level, + // we need to go up from it until we reach the current region's level + while (NewLevel > CurrentLevel) { + NewPosRegion = NewPosRegion->getParentRegion(); + NewLevel--; + } + + // We can now check if the current region is identical to the + // parent region of the new position + while (CurrentRegion != NewPosRegion) { + // If they are not the same, we need to go up from both of them + // until we reach the common ancestor region + CurrentRegion = CurrentRegion->getParentRegion(); + NewPosRegion = NewPosRegion->getParentRegion(); + CurrentLevel--; + } + + // Now we can find the common ancestor operation that is the closest to the + // current variable location by walking up the tree until we match the + // nesting level + mlir::Operation *CurrentOp = VarLoc.Position.getOperation(); + for (unsigned i = 0; i < (VarLoc.NestingLevel - CurrentLevel); ++i) { + CurrentOp = CurrentOp->getParentOp(); + } + + // Update the variable location to the newfound common ancestor + VarLoc.Position = clift::BlockPosition::get(CurrentOp); + VarLoc.NestingLevel = CurrentLevel; +} + +struct TightenVariableScopePass + : clift::impl::CliftTightenVariableScopesBase { + void runOnOperation() override { + clift::FunctionOp FunctionOp = getOperation(); + + // Store the function's local variables in a map, associated with their + // optimal position in the MLIR tree + llvm::SmallDenseMap + LocalVariables; + + auto WalkCallback = [&](mlir::Operation *Op, unsigned OpNestingLevel) { + // For each operand of the operation, we check if it is a local + // variable. + for (mlir::Value Operand : Op->getOperands()) { + auto LocalVarOp = Operand.getDefiningOp(); + + // If the defining op is not a local variable, we can skip it + if (not LocalVarOp) { + continue; + } + + // Local variables with an initializer cannot be moved + if (not LocalVarOp.getInitializer().empty()) { + continue; + } + + auto [Iterator, Inserted] = LocalVariables.try_emplace(LocalVarOp); + + if (Inserted) { + // If the local variable was not already in the map, mark + // its optimal position as right before the parent operation + // of the current user. + Iterator->second + .Position = clift::BlockPosition::get(Op->getParentOp()); + Iterator->second.NestingLevel = OpNestingLevel - 1; + } else { + // If the local variable is already in the map, update its + // location to find the common ancestor position that covers + // the new user + auto OpPosition = clift::BlockPosition::get(Op); + updateVariableLocation(Iterator->second, OpPosition, OpNestingLevel); + } + } + }; + + // Walk the function body to identify local variable uses, along with their + // nesting levels. + walkWithNestingLevel(&FunctionOp.getBody(), WalkCallback); + + // Move each local variable to its optimal position + for (const auto &[LocalVarOp, Location] : LocalVariables) { + // Get the target operation where we want to insert the variable + mlir::Operation *TargetOp = Location.Position.getOperation(); + + // Check if moving would be a no-op (already in the right place) + if (TargetOp->getPrevNode() == LocalVarOp) { + continue; + } + + // Move the local variable declaration to the optimal position + LocalVarOp->moveBefore(TargetOp); + } + } +}; + +} // namespace + +clift::PassPtr clift::createTightenVariableScopePass() { + return std::make_unique(); +} diff --git a/lib/mlir/Dialect/Clift/Transforms/CMakeLists.txt b/lib/mlir/Dialect/Clift/Transforms/CMakeLists.txt index 2110c81bfb..080fb7e9ac 100644 --- a/lib/mlir/Dialect/Clift/Transforms/CMakeLists.txt +++ b/lib/mlir/Dialect/Clift/Transforms/CMakeLists.txt @@ -12,6 +12,7 @@ add_mlir_dialect_library( Beautify/LoopDetection.cpp Beautify/Statements.cpp Beautify/ReturnConversion.cpp + Beautify/TightenVariableScopes.cpp Beautify.cpp CSemantics.cpp DEPENDS diff --git a/tests/unit/mlir_lit_tests/statement-rewrites/scope-tightening.mlir b/tests/unit/mlir_lit_tests/statement-rewrites/scope-tightening.mlir new file mode 100644 index 0000000000..ab8dfd9a82 --- /dev/null +++ b/tests/unit/mlir_lit_tests/statement-rewrites/scope-tightening.mlir @@ -0,0 +1,195 @@ +// +// This file is distributed under the MIT License. See LICENSE.md for details. +// + +// RUN: %revngcliftopt %s --tighten-variable-scopes | FileCheck %s + +!void = !clift.primitive +!int32_t = !clift.primitive + +!f = !clift.func< + "/type-definition/1001-CABIFunctionDefinition" : !void() +> + +module attributes {clift.module} { + clift.func @f1() { + // CHECK: [[L1:%[0-9]+]] = clift.local !int32_t + %0 = clift.local !int32_t + // CHECK-NOT: clift.local + %1 = clift.local !int32_t + %2 = clift.local !int32_t + %3 = clift.local !int32_t + + // CHECK: clift.if { + clift.if { + %4 = clift.undef : !int32_t + clift.yield %4 : !int32_t + // CHECK: } { + } { + clift.expr { + clift.yield %0 : !int32_t + } + // CHECK: } else { + } else { + // CHECK: clift.expr { + clift.expr { + clift.yield %0 : !int32_t + // CHECK: } + } + // CHECK: [[L2:%[0-9]+]] = clift.local !int32_t + // CHECK: clift.expr { + clift.expr { + // CHECK: clift.yield [[L2]] : !int32_t + clift.yield %1 : !int32_t + // CHECK: } + } + // CHECK: clift.if { + clift.if { + %5 = clift.undef : !int32_t + clift.yield %5 : !int32_t + // CHECK: } { + } { + // CHECK: [[L3:%[0-9]+]] = clift.local !int32_t + // CHECK: clift.expr { + clift.expr { + // CHECK: clift.yield [[L3]] : !int32_t + clift.yield %2 : !int32_t + // CHECK: } + } + // CHECK: } + } + // CHECK: } + } + // CHECK: [[L4:%[0-9]+]] = clift.local !int32_t + // CHECK: clift.expr { + clift.expr { + // CHECK: clift.yield [[L4]] : !int32_t + clift.yield %3 : !int32_t + // CHECK: } + } + // CHECK: } + } + + clift.func @f2() { + // CHECK: [[L1:%[0-9]+]] = clift.local !int32_t + %0 = clift.local !int32_t + // CHECK: [[L2:%[0-9]+]] = clift.local !int32_t + %1 = clift.local !int32_t + // CHECK-NOT: clift.local + %2 = clift.local !int32_t + + // CHECK: clift.if { + clift.if { + clift.yield %0 : !int32_t + // CHECK: } { + } { + // CHECK: clift.expr { + clift.expr { + clift.yield %1 : !int32_t + // CHECK: } + } + // CHECK: [[L3:%[0-9]+]] = clift.local !int32_t + // CHECK: clift.expr { + clift.expr { + // CHECK: clift.yield [[L3]] : !int32_t + clift.yield %2 : !int32_t + // CHECK: } + } + // CHECK: } else { + } else { + clift.expr { + clift.yield %1 : !int32_t + // CHECK: } + } + // CHECK: } + } + // CHECK: } + } + + clift.func @f3() { + // CHECK-NOT: clift.local + %0 = clift.local !int32_t + // CHECK: clift.expr { + clift.expr { + %1 = clift.imm 1 : !int32_t + clift.yield %1 : !int32_t + // CHECK: } + } + // CHECK: [[L:%[0-9]+]] = clift.local !int32_t + // CHECK: clift.if { + clift.if { + %2 = clift.imm 1 : !int32_t + clift.yield %2 : !int32_t + // CHECK: } { + } { + // CHECK: clift.if { + clift.if { + %3 = clift.imm 1 : !int32_t + clift.yield %3 : !int32_t + // CHECK: } { + } { + // CHECK: clift.expr { + clift.expr { + // CHECK: clift.yield [[L]] : !int32_t + clift.yield %0 : !int32_t + // CHECK: } + } + // CHECK: } + } + // CHECK: } + } + // CHECK: clift.if { + clift.if { + // CHECK: clift.yield [[L]] : !int32_t + clift.yield %0 : !int32_t + // CHECK: } { + } { + // CHECK: } + } + // CHECK: } + } + + clift.func @f4() { + // CHECK-NOT: clift.local + %0 = clift.local !int32_t + + // CHECK: clift.expr { + clift.expr { + %1 = clift.imm 1 : !int32_t + clift.yield %1 : !int32_t + // CHECK: } + } + // CHECK: [[L:%[0-9]+]] = clift.local !int32_t + // CHECK: clift.if { + clift.if { + // CHECK: clift.yield [[L]] : !int32_t + clift.yield %0 : !int32_t + // CHECK: } { + } { + // CHECK: } + } + // CHECK: clift.if { + clift.if { + %2 = clift.imm 1 : !int32_t + clift.yield %2 : !int32_t + // CHECK: } { + } { + // CHECK: clift.if { + clift.if { + %3 = clift.imm 1 : !int32_t + clift.yield %3 : !int32_t + // CHECK: } { + } { + // CHECK: clift.expr { + clift.expr { + // CHECK: clift.yield [[L]] : !int32_t + clift.yield %0 : !int32_t + // CHECK: } + } + // CHECK: } + } + // CHECK: } + } + // CHECK: } + } +}