[mlir][emitc] Isolate expressions from above #155641
Merged
+256
−122
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-emitc Author: Gil Rapaport (aniragil) ChangesThe expression op is currently not isolated from above. This served its original usage as an optional, translation-oriented op, but is becoming less convenient now that expressions appear earlier in the emitc compilation flow and are gaining use as components of other emitc ops. This patch therefore adds the isolated-from-above trait to expressions. Syntactically, the only change is in the expression's signature which now includes the values being used in the expression as arguments and their types. The region's argument's names shadow the used values to keep the def-use relations clear. Patch is 42.80 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/155641.diff 13 Files Affected:
diff --git a/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td b/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td
index 59beac7d64154..600e9e4574233 100644
--- a/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td
+++ b/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td
@@ -453,8 +453,8 @@ def EmitC_DivOp : EmitC_BinaryOp<"div", []> {
}
def EmitC_ExpressionOp : EmitC_Op<"expression",
- [HasOnlyGraphRegion, OpAsmOpInterface,
- SingleBlockImplicitTerminator<"emitc::YieldOp">, NoRegionArguments]> {
+ [HasOnlyGraphRegion, OpAsmOpInterface, IsolatedFromAbove,
+ SingleBlockImplicitTerminator<"emitc::YieldOp">]> {
let summary = "Expression operation";
let description = [{
The `emitc.expression` operation returns a single SSA value which is yielded by
@@ -491,12 +491,13 @@ def EmitC_ExpressionOp : EmitC_Op<"expression",
at its use.
}];
- let arguments = (ins UnitAttr:$do_not_inline);
+ let arguments = (ins Variadic<AnyTypeOf<[EmitCType, EmitC_LValueType]>>:$defs,
+ UnitAttr:$do_not_inline);
let results = (outs EmitCType:$result);
let regions = (region SizedRegion<1>:$region);
let hasVerifier = 1;
- let assemblyFormat = "attr-dict (`noinline` $do_not_inline^)? `:` type($result) $region";
+ let hasCustomAssemblyFormat = 1;
let extraClassDeclaration = [{
bool hasSideEffects() {
@@ -507,6 +508,13 @@ def EmitC_ExpressionOp : EmitC_Op<"expression",
return llvm::any_of(getRegion().front().without_terminator(), predicate);
};
Operation *getRootOp();
+ Block &createBody() {
+ assert(getRegion().empty() && "expression already has a body");
+ Block &block = getRegion().emplaceBlock();
+ for (auto operand : getOperands())
+ block.addArgument(operand.getType(), operand.getLoc());
+ return block;
+ }
//===------------------------------------------------------------------===//
// OpAsmOpInterface Methods
diff --git a/mlir/lib/Conversion/ArithToEmitC/ArithToEmitC.cpp b/mlir/lib/Conversion/ArithToEmitC/ArithToEmitC.cpp
index 515fe5c9980c6..b68933d0fb0a0 100644
--- a/mlir/lib/Conversion/ArithToEmitC/ArithToEmitC.cpp
+++ b/mlir/lib/Conversion/ArithToEmitC/ArithToEmitC.cpp
@@ -610,16 +610,19 @@ class ShiftOpConversion : public OpConversionPattern<ArithOp> {
? rewriter.getIntegerAttr(arithmeticType, 0)
: rewriter.getIndexAttr(0)));
- emitc::ExpressionOp ternary = emitc::ExpressionOp::create(
- rewriter, op.getLoc(), arithmeticType, /*do_not_inline=*/false);
- Block &bodyBlock = ternary.getBodyRegion().emplaceBlock();
+ emitc::ExpressionOp ternary =
+ emitc::ExpressionOp::create(rewriter, op.getLoc(), arithmeticType,
+ ValueRange({lhs, rhs, excessCheck, poison}),
+ /*do_not_inline=*/false);
+ Block &bodyBlock = ternary.createBody();
auto currentPoint = rewriter.getInsertionPoint();
rewriter.setInsertionPointToStart(&bodyBlock);
Value arithmeticResult =
- EmitCOp::create(rewriter, op.getLoc(), arithmeticType, lhs, rhs);
- Value resultOrPoison =
- emitc::ConditionalOp::create(rewriter, op.getLoc(), arithmeticType,
- excessCheck, arithmeticResult, poison);
+ EmitCOp::create(rewriter, op.getLoc(), arithmeticType,
+ bodyBlock.getArgument(0), bodyBlock.getArgument(1));
+ Value resultOrPoison = emitc::ConditionalOp::create(
+ rewriter, op.getLoc(), arithmeticType, bodyBlock.getArgument(2),
+ arithmeticResult, bodyBlock.getArgument(3));
emitc::YieldOp::create(rewriter, op.getLoc(), resultOrPoison);
rewriter.setInsertionPoint(op->getBlock(), currentPoint);
diff --git a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
index 5359826eed0fd..ad93c4b15d285 100644
--- a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
+++ b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
@@ -375,6 +375,52 @@ OpFoldResult emitc::ConstantOp::fold(FoldAdaptor adaptor) { return getValue(); }
// ExpressionOp
//===----------------------------------------------------------------------===//
+ParseResult ExpressionOp::parse(OpAsmParser &parser, OperationState &result) {
+ SmallVector<OpAsmParser::UnresolvedOperand> operands;
+ if (parser.parseOperandList(operands))
+ return parser.emitError(parser.getCurrentLocation()) << "expected operands";
+ if (succeeded(parser.parseOptionalKeyword("noinline")))
+ result.addAttribute(ExpressionOp::getDoNotInlineAttrName(result.name),
+ parser.getBuilder().getUnitAttr());
+ Type type;
+ if (parser.parseColonType(type))
+ return parser.emitError(parser.getCurrentLocation(),
+ "expected function type");
+ auto fnType = llvm::dyn_cast<FunctionType>(type);
+ if (!fnType)
+ return parser.emitError(parser.getCurrentLocation(),
+ "expected function type");
+ if (parser.resolveOperands(operands, fnType.getInputs(),
+ parser.getCurrentLocation(), result.operands))
+ return failure();
+ if (fnType.getNumResults() != 1)
+ return parser.emitError(parser.getCurrentLocation(),
+ "expected single return type");
+ result.addTypes(fnType.getResults());
+ Region *body = result.addRegion();
+ SmallVector<OpAsmParser::Argument> argsInfo;
+ for (auto [unresolvedOperand, operandType] :
+ llvm::zip(operands, fnType.getInputs())) {
+ OpAsmParser::Argument argInfo;
+ argInfo.ssaName = unresolvedOperand;
+ argInfo.type = operandType;
+ argsInfo.push_back(argInfo);
+ }
+ if (parser.parseRegion(*body, argsInfo, /*enableNameShadowing=*/true))
+ return failure();
+ return success();
+}
+
+void emitc::ExpressionOp::print(OpAsmPrinter &p) {
+ p << ' ';
+ p.printOperands(getDefs());
+ p << " : ";
+ p.printFunctionalType(getOperation());
+ p.shadowRegionArgs(getRegion(), getDefs());
+ p << ' ';
+ p.printRegion(getRegion(), /*printEntryBlockArgs=*/false);
+}
+
Operation *ExpressionOp::getRootOp() {
auto yieldOp = cast<YieldOp>(getBody()->getTerminator());
Value yieldedValue = yieldOp.getResult();
diff --git a/mlir/lib/Dialect/EmitC/Transforms/Transforms.cpp b/mlir/lib/Dialect/EmitC/Transforms/Transforms.cpp
index 3f0690c3b07ad..c999f78a76371 100644
--- a/mlir/lib/Dialect/EmitC/Transforms/Transforms.cpp
+++ b/mlir/lib/Dialect/EmitC/Transforms/Transforms.cpp
@@ -9,7 +9,9 @@
#include "mlir/Dialect/EmitC/Transforms/Transforms.h"
#include "mlir/Dialect/EmitC/IR/EmitC.h"
#include "mlir/IR/IRMapping.h"
+#include "mlir/IR/Location.h"
#include "mlir/IR/PatternMatch.h"
+#include "llvm/ADT/STLExtras.h"
namespace mlir {
namespace emitc {
@@ -24,20 +26,24 @@ ExpressionOp createExpression(Operation *op, OpBuilder &builder) {
Location loc = op->getLoc();
builder.setInsertionPointAfter(op);
- auto expressionOp = emitc::ExpressionOp::create(builder, loc, resultType);
+ auto expressionOp =
+ emitc::ExpressionOp::create(builder, loc, resultType, op->getOperands());
// Replace all op's uses with the new expression's result.
result.replaceAllUsesWith(expressionOp.getResult());
- // Create an op to yield op's value.
- Region ®ion = expressionOp.getRegion();
- Block &block = region.emplaceBlock();
+ Block &block = expressionOp.createBody();
+ IRMapping mapper;
+ for (auto [operand, arg] : llvm::zip(expressionOp.getOperands(),
+ block.getArguments()))
+ mapper.map(operand, arg);
builder.setInsertionPointToEnd(&block);
- auto yieldOp = emitc::YieldOp::create(builder, loc, result);
- // Move op into the new expression.
- op->moveBefore(yieldOp);
+ Operation *rootOp = builder.clone(*op, mapper);
+ op->erase();
+ // Create an op to yield op's value.
+ emitc::YieldOp::create(builder, loc, rootOp->getResults()[0]);
return expressionOp;
}
@@ -53,51 +59,93 @@ struct FoldExpressionOp : public OpRewritePattern<ExpressionOp> {
using OpRewritePattern<ExpressionOp>::OpRewritePattern;
LogicalResult matchAndRewrite(ExpressionOp expressionOp,
PatternRewriter &rewriter) const override {
- bool anythingFolded = false;
- for (Operation &op : llvm::make_early_inc_range(
- expressionOp.getBody()->without_terminator())) {
- // Don't fold expressions whose result value has its address taken.
- auto applyOp = dyn_cast<emitc::ApplyOp>(op);
- if (applyOp && applyOp.getApplicableOperator() == "&")
- continue;
-
- for (Value operand : op.getOperands()) {
- auto usedExpression = operand.getDefiningOp<ExpressionOp>();
- if (!usedExpression)
- continue;
-
- // Don't fold expressions with multiple users: assume any
- // re-materialization was done separately.
- if (!usedExpression.getResult().hasOneUse())
- continue;
-
- // Don't fold expressions with side effects.
- if (usedExpression.hasSideEffects())
- continue;
-
- // Fold the used expression into this expression by cloning all
- // instructions in the used expression just before the operation using
- // its value.
- rewriter.setInsertionPoint(&op);
- IRMapping mapper;
- for (Operation &opToClone :
- usedExpression.getBody()->without_terminator()) {
- Operation *clone = rewriter.clone(opToClone, mapper);
- mapper.map(&opToClone, clone);
- }
-
- Operation *expressionRoot = usedExpression.getRootOp();
- Operation *clonedExpressionRootOp = mapper.lookup(expressionRoot);
- assert(clonedExpressionRootOp &&
- "Expected cloned expression root to be in mapper");
- assert(clonedExpressionRootOp->getNumResults() == 1 &&
- "Expected cloned root to have a single result");
-
- rewriter.replaceOp(usedExpression, clonedExpressionRootOp);
- anythingFolded = true;
- }
+ Block *expressionBody = expressionOp.getBody();
+ ExpressionOp usedExpression;
+ SetVector<Value> foldedOperands;
+
+ auto takesItsOperandsAddress = [](Operation *user) {
+ auto applyOp = dyn_cast<emitc::ApplyOp>(user);
+ return applyOp && applyOp.getApplicableOperator() == "&";
+ };
+
+ // Select as expression to fold the first operand expression that
+ // - doesn't have its result value's address taken,
+ // - has a single user: assume any re-materialization was done separately,
+ // - has no side effects,
+ // and save all other operands to be used later as operands in the folded
+ // expression.
+ for (auto [operand, arg] : llvm::zip(expressionOp.getOperands(),
+ expressionBody->getArguments())) {
+ ExpressionOp operandExpression = operand.getDefiningOp<ExpressionOp>();
+ if (usedExpression || !operandExpression ||
+ llvm::any_of(arg.getUsers(), takesItsOperandsAddress) ||
+ !operandExpression.getResult().hasOneUse() ||
+ operandExpression.hasSideEffects())
+ foldedOperands.insert(operand);
+ else
+ usedExpression = operandExpression;
}
- return anythingFolded ? success() : failure();
+
+ // If no operand expression was selected, bail out.
+ if (!usedExpression)
+ return failure();
+
+ // Collect additional operands from the folded expression.
+ for (Value operand : usedExpression.getOperands())
+ foldedOperands.insert(operand);
+
+ // Create a new expression to hold the folding result.
+ rewriter.setInsertionPointAfter(expressionOp);
+ auto foldedExpression = emitc::ExpressionOp::create(
+ rewriter, expressionOp.getLoc(), expressionOp.getResult().getType(),
+ foldedOperands.getArrayRef(), expressionOp.getDoNotInline());
+ Block &foldedExpressionBody = foldedExpression.createBody();
+
+ // Map each operand of the new expression to its matching block argument.
+ IRMapping mapper;
+ for (auto [operand, arg] : llvm::zip(foldedExpression.getOperands(),
+ foldedExpressionBody.getArguments()))
+ mapper.map(operand, arg);
+
+ // Prepare to fold the used expression and the matched expression into the
+ // newly created folded expression.
+ auto foldExpression = [&rewriter, &mapper](ExpressionOp expressionToFold,
+ bool withTerminator) {
+ Block *expressionToFoldBody = expressionToFold.getBody();
+ for (auto [operand, arg] :
+ llvm::zip(expressionToFold.getOperands(),
+ expressionToFoldBody->getArguments())) {
+ mapper.map(arg, mapper.lookup(operand));
+ }
+
+ for (Operation &opToClone : expressionToFoldBody->without_terminator())
+ rewriter.clone(opToClone, mapper);
+
+ if (withTerminator)
+ rewriter.clone(*expressionToFoldBody->getTerminator(), mapper);
+ };
+ rewriter.setInsertionPointToStart(&foldedExpressionBody);
+
+ // First, fold the used expression into the new expression and map its
+ // result to the clone of its root operation within the new expression.
+ foldExpression(usedExpression, /*withTerminator=*/false);
+ Operation *expressionRoot = usedExpression.getRootOp();
+ Operation *clonedExpressionRootOp = mapper.lookup(expressionRoot);
+ assert(clonedExpressionRootOp &&
+ "Expected cloned expression root to be in mapper");
+ assert(clonedExpressionRootOp->getNumResults() == 1 &&
+ "Expected cloned root to have a single result");
+ mapper.map(usedExpression.getResult(),
+ clonedExpressionRootOp->getResults()[0]);
+
+ // Now fold the matched expression into the new expression.
+ foldExpression(expressionOp, /*withTerminator=*/true);
+
+ // Complete the rewrite.
+ rewriter.replaceOp(expressionOp, foldedExpression);
+ rewriter.eraseOp(usedExpression);
+
+ return success();
}
};
diff --git a/mlir/lib/Target/Cpp/TranslateToCpp.cpp b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
index 7284e24776175..6787c97340bd6 100644
--- a/mlir/lib/Target/Cpp/TranslateToCpp.cpp
+++ b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
@@ -14,6 +14,7 @@
#include "mlir/IR/Dialect.h"
#include "mlir/IR/Operation.h"
#include "mlir/IR/SymbolTable.h"
+#include "mlir/IR/Value.h"
#include "mlir/Support/IndentedOstream.h"
#include "mlir/Support/LLVM.h"
#include "mlir/Target/Cpp/CppEmitter.h"
@@ -364,9 +365,10 @@ static bool shouldBeInlined(ExpressionOp expressionOp) {
if (hasDeferredEmission(user))
return false;
- // Do not inline expressions used by ops with the CExpressionInterface. If
- // this was intended, the user could have been merged into the expression op.
- return !isa<emitc::CExpressionInterface>(*user);
+ // Do not inline expressions used by other expressions or by ops with the
+ // CExpressionInterface. If this was intended, the user could have been merged
+ // into the expression op.
+ return !isa<emitc::ExpressionOp, emitc::CExpressionInterface>(*user);
}
static LogicalResult printConstantOp(CppEmitter &emitter, Operation *operation,
@@ -1532,6 +1534,20 @@ LogicalResult CppEmitter::emitOperand(Value value) {
if (expressionOp && shouldBeInlined(expressionOp))
return emitExpression(expressionOp);
+ if (BlockArgument arg = dyn_cast<BlockArgument>(value)) {
+ // If this operand is a block argument of an expression, emit instead the
+ // matching expression parameter.
+ Operation *argOp = arg.getParentBlock()->getParentOp();
+ if (auto expressionOp = dyn_cast<ExpressionOp>(argOp)) {
+ // This scenario is only expected when one of the operations within the
+ // expression being emitted references one of the expression's block
+ // arguments.
+ assert(expressionOp == emittedExpression &&
+ "Expected expression being emitted");
+ value = expressionOp->getOperand(arg.getArgNumber());
+ }
+ }
+
os << getOrCreateName(value);
return success();
}
diff --git a/mlir/test/Conversion/ArithToEmitC/arith-to-emitc.mlir b/mlir/test/Conversion/ArithToEmitC/arith-to-emitc.mlir
index 1382f3cc13a3b..319dfc31ab637 100644
--- a/mlir/test/Conversion/ArithToEmitC/arith-to-emitc.mlir
+++ b/mlir/test/Conversion/ArithToEmitC/arith-to-emitc.mlir
@@ -153,7 +153,7 @@ func.func @arith_shift_left(%arg0: i32, %arg1: i32) {
// CHECK-DAG: %[[SizeConstant:[^ ]*]] = "emitc.constant"{{.*}}value = 32
// CHECK-DAG: %[[CmpNoExcess:[^ ]*]] = emitc.cmp lt, %[[C2]], %[[SizeConstant]] : (ui32, ui32) -> i1
// CHECK-DAG: %[[Zero:[^ ]*]] = "emitc.constant"{{.*}}value = 0
- // CHECK: %[[ShiftRes:[^ ]*]] = emitc.expression : ui32 {
+ // CHECK: %[[ShiftRes:[^ ]*]] = emitc.expression %[[C1]], %[[C2]], %[[CmpNoExcess]], %[[Zero]] : (ui32, ui32, i1, ui32) -> ui32 {
// CHECK-NEXT: %[[SHL:[^ ]*]] = bitwise_left_shift %[[C1]], %[[C2]] : (ui32, ui32) -> ui32
// CHECK-NEXT: %[[Ternary:[^ ]*]] = conditional %[[CmpNoExcess]], %[[SHL]], %[[Zero]] : ui32
// CHECK-NEXT: yield %[[Ternary]] : ui32
@@ -173,7 +173,7 @@ func.func @arith_shift_right(%arg0: i32, %arg1: i32) {
// CHECK-DAG: %[[SizeConstant:[^ ]*]] = "emitc.constant"{{.*}}value = 32{{.*}}ui32
// CHECK-DAG: %[[CmpNoExcess:[^ ]*]] = emitc.cmp lt, %[[C2]], %[[SizeConstant]] : (ui32, ui32) -> i1
// CHECK-DAG: %[[Zero:[^ ]*]] = "emitc.constant"{{.*}}value = 0{{.*}}ui32
- // CHECK: %[[ShiftRes:[^ ]*]] = emitc.expression : ui32 {
+ // CHECK: %[[ShiftRes:[^ ]*]] = emitc.expression %[[C1]], %[[C2]], %[[CmpNoExcess]], %[[Zero]] : (ui32, ui32, i1, ui32) -> ui32 {
// CHECK-NEXT: %[[SHR:[^ ]*]] = bitwise_right_shift %[[C1]], %[[C2]] : (ui32, ui32) -> ui32
// CHECK-NEXT: %[[Ternary:[^ ]*]] = conditional %[[CmpNoExcess]], %[[SHR]], %[[Zero]] : ui32
// CHECK-NEXT: yield %[[Ternary]] : ui32
@@ -185,7 +185,7 @@ func.func @arith_shift_right(%arg0: i32, %arg1: i32) {
// CHECK-DAG: %[[SSizeConstant:[^ ]*]] = "emitc.constant"{{.*}}value = 32{{.*}}ui32
// CHECK-DAG: %[[SCmpNoExcess:[^ ]*]] = emitc.cmp lt, %[[SC2]], %[[SSizeConstant]] : (ui32, ui32) -> i1
// CHECK-DAG: %[[SZero:[^ ]*]] = "emitc.constant"{{.*}}value = 0{{.*}}i32
- // CHECK: %[[SShiftRes:[^ ]*]] = emitc.expression : i32 {
+ // CHECK: %[[SShiftRes:[^ ]*]] = emitc.expression %[[ARG0]], %[[SC2]], %[[SCmpNoExcess]], %[[SZero]] : (i32, ui32, i1, i32) -> i32 {
// CHECK-NEXT: %[[SHRSI:[^ ]*]] = bitwise_right_shift %[[ARG0]], %[[SC2]] : (i32, ui32) -> i32
// CHECK-NEXT: %[[STernary:[^ ]*]] = conditional %[[SCmpNoExcess]], %[[SHRSI]], %[[SZero]] : i32
// CHECK-NEXT: yield %[[STernary]] : i32
@@ -210,7 +210,7 @@ func.func @arith_shift_left_index(%amount: i32) {
// CHECK-DAG: %[[SizeConstant:[^ ]*]] = emitc.mul %[[Byte]], %[[SizeOf]] : (!emitc.size_t, !emitc.size_t) -> !emitc.size_t
// CHECK-DAG: %[[CmpNoExcess:[^ ]*]] = emitc.cmp lt, %[[AmountIdx]], %[[SizeConstant]] : (!emitc.size_t, !emitc.size_t) -> i1
// CHECK-DAG: %[[Zero:[^ ]*]] = "emitc.constant"{{.*}}value = 0
- // CHECK: %[[ShiftRes:[^ ]*]] = emitc.expression : !emitc.size_t {
+ // CHECK: %[[ShiftRes:[^ ]*]] = emitc.expression %[[C1]], %[[AmountIdx]], %[[CmpNoExcess]], %[[Zero]] : (!emitc.size_t, !emitc.size_t, i1, !emitc.size_t) -> !emitc.size_t {
// CHECK-NEXT: %[[SHL:[^ ]*]] = bitwise_left_shift %[[C1]], %[[AmountIdx]] : (!emitc.size_t, !emitc.size_t) -> !emitc.size_t
// CHECK-NEXT: %[[Ternary:[^ ]*]] = conditional %[[CmpNoExcess]], %[[SHL]], %[[Zero]] : !emitc.size_t
// CHECK-NEXT: yield %[[Ternary]] : !emitc.size_t
@@ -235,7 +235,7 @@ func.func @arith_shift_right_index(%amount: i32) {
// CHECK-DAG: %[[SizeConstant:[^ ]*]] = emitc.mul %[[Byte]], %[[SizeOf]] : (!emitc.size_t, !emitc.size_t) -> !emitc.size_t
// CHECK-DAG: %[[CmpNoExcess:[^ ]*]] = emitc.cmp lt, %[[AmountIdx]], %[[SizeConstant]] : (!emitc.size_t, !emitc.size_t) -> i1
// CHECK-DAG: %[[Zero:[^ ]*]] = "emitc.constant"{{.*}}value = 0{{.*}}!emitc.size_t
- // CHECK: %[[ShiftRes:[^ ]*]] = emitc.expression : !emitc.size_t {
+ // CHECK: %[[ShiftRes:[^ ]*]] = emitc.expression %[[C1]], %[[AmountIdx]], %[[CmpNoExcess]], %[[Zero]] : (!emitc.size_t, !emitc.size_t, i1, !emitc.size_t) -> !emitc.size_t {
// CHECK-NEXT: %[[SHR:[^ ]*]] = bitwise_right_shift %[[C1]], %[[AmountIdx]] : (!emitc....
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Thanks for the clarification and for making the change! You are absolutely right with adding the |
|
What do you think about making syntax more function like? For example: emitc.expression(%a : i1, %b : i1) -> i1 {
} |
The |
No super strong feelings on my side either but maybe a slight preference to keep as is. expression %a, %b : (i1, i1) -> i1 {..}is closer to other operations while |
|
I mostly suggested that, because it's isolated from above, so it'll be a bit more clear, since it essentially acts as a function/graph region from what I can see, when it comes to values inside of it. |
I think @marbre raises a good point: functions define their parameters while other ops (including calls) take arguments, as do expressions (due to name shadowing expressions don't introduce any new names). |
simon-camp
approved these changes
Aug 31, 2025
marbre
approved these changes
Sep 1, 2025
The expression op is currently not isolated from above. This served its original usage as an optional, translation-oriented op, but is becoming less convenient now that expressions appear earlier in the emitc compilation flow and are gaining use as components of other emitc ops. This patch therefore adds the isolated-from-above trait to expressions. Syntactically, the only change is in the expression's signature which now includes the values being used in the expression as arguments and their types. The region's argument's names shadow the used values to keep the def-use relations clear.
c9ffdb1 to
c618df8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The expression op is currently not isolated from above. This served its original usage as an optional, translation-oriented op, but is becoming less convenient now that expressions appear earlier in the emitc compilation flow and are gaining use as components of other emitc ops.
This patch therefore adds the isolated-from-above trait to expressions. Syntactically, the only change is in the expression's signature which now includes the values being used in the expression as arguments and their types. The region's argument's names shadow the used values to keep the def-use relations clear.