[Transform] Add convert-index-to-uint transform to normalize index compares before comb mapping #9263
Conversation
|
@cowardsa This is another transform we have in our flow to go down to datapath dialect. Therefore I try to push it to upstream circt ! Thanks for looking at it. |
cowardsa
left a comment
cowardsa
left a comment
There was a problem hiding this comment.
Mostly looks good - cool to see this pipeline progressing!
Would just split the tests please so we can isolate testing of the newly added pass as otherwise it could mask bugs.
| @@ -0,0 +1,154 @@ | |||
| //===- ConvertIndexToUInt.cpp - Rewrite index compares to integers --------===// | |||
There was a problem hiding this comment.
Nit: this header format is no longer required - can just use:
//===----------------------------------------------------------------------===//
| // | ||
| //===----------------------------------------------------------------------===// | ||
| // | ||
| // Contains the definitions of the ConvertIndexToUInt pass. |
There was a problem hiding this comment.
Nit: slightly redundant comment given the filename? I also think these are not entirely necessary if you see for example the datapath dialect files they do not have these comments
| if (!op.getLhs().getType().isIndex()) | ||
| return failure(); |
There was a problem hiding this comment.
Nit: worth also checking Rhs? Or adding an assertion if you get past the lhs check that RHS is index type?
| return failure(); | ||
|
|
||
| auto convertOperand = [&](Value operand) -> FailureOr<Value> { | ||
| if (auto castOp = operand.getDefiningOp<arith::IndexCastOp>()) { |
There was a problem hiding this comment.
Nit: can we add a comment explaining what this block is matching and doing?
| Replace `arith.cmpi` operations whose operands are `index` values (often | ||
| produced when lowering `scf.index_switch`) with comparisons over the | ||
| original integer type so that downstream hardware mapping passes (e.g. | ||
| `--map-arith-to-comb`) do not encounter unsupported index-typed arithmetic. | ||
| The pass converts any associated index constants and erases the redundant | ||
| casts that become dead afterwards. |
There was a problem hiding this comment.
Nit: a concrete example potentially one from the tests would help to understand what transformation is performed?
| @@ -0,0 +1,81 @@ | |||
| // RUN: circt-opt -split-input-file --switch-to-if --convert-index-to-uint --canonicalize %s | FileCheck %s | |||
There was a problem hiding this comment.
In general CIRCT tries to avoid chaining passes in tests - can you split this into two tests (possibly in the same file as is done here integration_test/circt-lec/comb.mlir)? That way we can isolate testing of convert-index-to-uint?
| std::unique_ptr<mlir::Pass> circt::createConvertIndexToUIntPass() { | ||
| return std::make_unique<ConvertIndexToUIntPass>(); | ||
| } |
There was a problem hiding this comment.
@uenoku - these definitions get automatically generated now right?
There was a problem hiding this comment.
It's not automatically generated if there is " let constructor = "circt::createConvertIndexToUIntPass()"; in pass definition so the current implementation looks good to me.
| %0 = scf.index_switch %switch_val -> i1 | ||
| case 5 { | ||
| %t = arith.constant true | ||
| scf.yield %t : i1 | ||
| } | ||
| default { | ||
| %f = arith.constant false | ||
| scf.yield %f : i1 | ||
| } | ||
| return %0 : i1 |
There was a problem hiding this comment.
Once tests are split this will probably need to use different CHECK-*** labels - see examples in circt-lec tests of how to specify multiple filecheck labels
| // CHECK: %[[RES:.*]] = scf.if %[[CMP0]] -> (i1) { | ||
| // CHECK: } else { |
There was a problem hiding this comment.
Q: Is there a missing yield line here? for the return of the if branch?
|
I think inferring bitwidth for index based on cast(consant(c)) pattern seems to be a bit restrictive. As mentioned in #5297 it may be a better direction to lower index type to a specific bit width, and later narrow the width in comb dialect. WDYT?
|
3 participants
Implemented a convert-index-to-uint transform pass to rewrite index-typed arith.cmpi/constants back to their originating integer type (removing redundant arith.index_cast and dead index constants). This unblocks map-arith-to-comb and other hardware mappings that can’t legalize index arithmetic, e.g. after lowering scf.index_switch or any flow that leaves index compares.
Added pass definition/registration in include/circt/Transforms/Passes.{td,h} and implementation in lib/Transforms/ConvertIndexToUInt.cpp; wired into the transforms CMake.
New lit coverage (test/Transforms/convert-index-to-uint.mlir) exercises both plain index compares and switch-lowering cases to ensure index types disappear and compares run on the original integer widths.
Tests:
ninja -C build circt-opt
llvm-lit build/test/Transforms/convert-index-to-uint.mlir