New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dialects: (affine) add result_types to For from_region helper #1195
Conversation
@@ -69,14 +70,13 @@ def from_region( | |||
upper_bound = IntegerAttr.from_index_int_value(upper_bound) | |||
if isinstance(step, int): | |||
step = IntegerAttr.from_index_int_value(step) | |||
result_types = [SSAValue.get(op).typ for op in operands] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that this is not always the case, the MLIR For builder only has the iteration value types as the result types, not the lb/ub value types, but they are all operands
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you referring to
for (Value val : iterArgs)
result.addTypes(val.getType());
from AffineForOp::build
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, I'm not really sure that they're equivalent but it seems that they are? I've not actually used the CPP version before
8b6860f
to
73b1fe5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a little test where these actually differ?
I'm not sure how to test this tbh |
73b1fe5
to
e8f9b2e
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1195 +/- ##
==========================================
- Coverage 88.96% 88.96% -0.01%
==========================================
Files 166 166
Lines 22488 22487 -1
Branches 3404 3402 -2
==========================================
- Hits 20007 20006 -1
Misses 1937 1937
Partials 544 544
☔ View full report in Codecov by Sentry. |
Just so you know I have tested this by roundtripping this through MLIR on my machine |
Just no unit/filecheck tests |
//===----------------------------------------------------------------------===//
// AffineForOp
//===----------------------------------------------------------------------===//
/// 'bodyBuilder' is used to build the body of affine.for. If iterArgs and
/// bodyBuilder are empty/null, we include default terminator op.
void AffineForOp::build(OpBuilder &builder, OperationState &result,
ValueRange lbOperands, AffineMap lbMap,
ValueRange ubOperands, AffineMap ubMap, int64_t step,
ValueRange iterArgs, BodyBuilderFn bodyBuilder) {
assert(((!lbMap && lbOperands.empty()) ||
lbOperands.size() == lbMap.getNumInputs()) &&
"lower bound operand count does not match the affine map");
assert(((!ubMap && ubOperands.empty()) ||
ubOperands.size() == ubMap.getNumInputs()) &&
"upper bound operand count does not match the affine map");
assert(step > 0 && "step has to be a positive integer constant");
for (Value val : iterArgs)
result.addTypes(val.getType());
// Add an attribute for the step.
result.addAttribute(getStepAttrStrName(),
builder.getIntegerAttr(builder.getIndexType(), step));
// Add the lower bound.
result.addAttribute(getLowerBoundAttrStrName(), AffineMapAttr::get(lbMap));
result.addOperands(lbOperands);
// Add the upper bound.
result.addAttribute(getUpperBoundAttrStrName(), AffineMapAttr::get(ubMap));
result.addOperands(ubOperands);
result.addOperands(iterArgs);
// Create a region and a block for the body. The argument of the region is
// the loop induction variable.
Region *bodyRegion = result.addRegion();
bodyRegion->push_back(new Block);
Block &bodyBlock = bodyRegion->front();
Value inductionVar =
bodyBlock.addArgument(builder.getIndexType(), result.location);
for (Value val : iterArgs)
bodyBlock.addArgument(val.getType(), val.getLoc());
// Create the default terminator if the builder is not provided and if the
// iteration arguments are not provided. Otherwise, leave this to the caller
// because we don't know which values to return from the loop.
if (iterArgs.empty() && !bodyBuilder) {
ensureTerminator(*bodyRegion, builder, result.location);
} else if (bodyBuilder) {
OpBuilder::InsertionGuard guard(builder);
builder.setInsertionPointToStart(&bodyBlock);
bodyBuilder(builder, result.location, inductionVar,
bodyBlock.getArguments().drop_front());
}
} |
@Groverkss