Comb Interval Range Analysis and Comb Opt Narrowing pass #8425

cowardsa · 2025-04-17T11:47:40Z

Building on the existing MLIR integer interval range analysis framework, build the interface for the comb dialect. Use the interval range analysis to develop a comb opt narrowing pass, that reduces a comb opt based on the interval range of the operation. Currently this is only supported for addition, subtraction and multiplication, but this could be extended in the future. Future work will leverage the interval analysis to validate the combination of consecutive addition operators into a multi-operand addition.

…implement anything - just returns unknown for every operator

…comb dialect.

…the truthtable and parity operator.

… just return the value of bits that can be removed.

…d add test for new pass.

…p implementations

TaoBi22

This looks pretty good to me! Mostly just nits, the only big thing is where this int range analysis should live (expanded on below). @uenoku since this is along the lines of synthesis, is there any chance you could take a quick look at it too?

lib/Dialect/Comb/Transforms/IntRangeOptimizations.cpp

lib/Dialect/Comb/InferIntRangeInterfaceImpls.cpp

TaoBi22 · 2025-04-17T11:59:07Z

lib/Dialect/Comb/InferIntRangeInterfaceImpls.cpp

@@ -0,0 +1,256 @@
+//===- InferIntRangeInterfaceImpls.cpp - Integer range impls for comb -===//


I think it's a little unusual for something like this to live in lib/Dialect/<dialect> since it goes beyond the actual dialect definition, maybe it would make more sense to have it in lib/Analysis?

Also, when creating an analysis we often create a test pass in this file that just runs the analysis and prints out the results in a checkable format, and then use this to create some FileCheck tests for the analysis. It would be nice to do that here too (and if this moves to Analysis then you could just add it in that file)

@TaoBi22 - agree with this suggestion - there's currently no lib/Analysis/Comb hierarchy and given that I've got an implementation for both HW and Comb would it be sensible to introduce the directory structure in lib/Analysis?

Note that in MLIR these files live here say, lib/Dialect/Arith/IR/InferIntRangeInterfaceImpls.cpp

I think the current location for these files is fine.
One could make an argument that this logic should be tied more to the transformation pass and less to the dialect definition in which case you could implement them as external models and move them to the Transforms folder (or a new Analysis folder). Both seem fine to me.

Cool, I don't have strong feelings on this so if it matches what's done upstream then feel free to ignore

Ok will stick with as is for now - if the space of analyses grows then will perhaps make sense to separate as per Bea's suggestion

@TaoBi22 and @uenoku - have added a testing pass as per suggestion that appends the interval analysis data as an operation attribute which we can then check using Filecheck - let me know if this is suitable? This includes testing for the mux interval analysis.

lib/Dialect/Comb/InferIntRangeInterfaceImpls.cpp

lib/Dialect/HW/InferIntRangeInterfaceImpls.cpp

lib/Dialect/HW/Transforms/CMakeLists.txt

test/Dialect/Comb/comb-int-range-narrowing.mlir

…matting corrections

maerhart

Nice work! 🎉
Just added some drive-by comments.

test/Dialect/Comb/comb-int-range-narrowing.mlir

lib/Dialect/Comb/Transforms/IntRangeOptimizations.cpp

lib/Dialect/HW/InferIntRangeInterfaceImpls.cpp

lib/Dialect/Comb/InferIntRangeInterfaceImpls.cpp

lib/Dialect/Comb/Transforms/IntRangeOptimizations.cpp

maerhart · 2025-04-21T09:03:38Z

lib/Dialect/Comb/InferIntRangeInterfaceImpls.cpp

@@ -0,0 +1,256 @@
+//===- InferIntRangeInterfaceImpls.cpp - Integer range impls for comb -===//


I think the current location for these files is fine.
One could make an argument that this logic should be tied more to the transformation pass and less to the dialect definition in which case you could implement them as external models and move them to the Transforms folder (or a new Analysis folder). Both seem fine to me.

lib/Dialect/Comb/InferIntRangeInterfaceImpls.cpp

uenoku

Thank you for interesting work! I think the interface is definitely worth to have 👍
High-level comments:

Is it possible to have a test for range interface decoupled from narrowing pass? I think it's a bit difficult to track the correctness of the analysis. i.e. we don't have a test for range interface tests of comb.mux etc.
Please use camelBack for variable names

lib/Dialect/Comb/InferIntRangeInterfaceImpls.cpp

lib/Dialect/Comb/Transforms/IntRangeOptimizations.cpp

…ctually executes the pass

cowardsa · 2025-04-24T13:42:27Z

@uenoku/@maerhart - have addressed all the comments above - would you have time to take another pass and approve if you feel its ready?

Major changes:

Formatting, newlines, camelBack etc corrected throughout
Added interval analysis test pass adding the analysis results as attributes to be checked
Made all tests use regex throughout and made each test only stress a single pass

uenoku

Thank you for adding a test pass. Could you add a test for replicate, extract and icmp? There is a correctness issue in add/mul pattern and crash with the following case, once after the issue was fixed PR look good to me.

hw.module private @InitReg1(in %clock: !seq.clock, in %reset: i1, in %io_d: i32, in %io_en: i1, out io_q: i32) {
  %false = hw.constant false
  %c1_i32 = hw.constant 1 : i32
  %c0_i32 = hw.constant 0 : i32
  %reg = seq.firreg %4 clock %clock sym @__reg__ reset async %reset, %c0_i32 : i32
  %reg2 = seq.firreg %reg2 clock %clock sym @__reg2__ reset sync %reset, %c0_i32 : i32
  %reg3 = seq.firreg %reg3 clock %clock sym @__reg3__ reset async %reset, %c1_i32 : i32
  %0 = comb.concat %false, %reg : i1, i32
  %1 = comb.concat %false, %reg2 : i1, i32
  %2 = comb.add %0, %1 : i33
  %3 = comb.extract %2 from 1 : (i33) -> i32
  %4 = comb.mux bin %io_en, %io_d, %3 : i32
  hw.output %reg : i32
}

circt-opt: /scratch/hidetou/circt/llvm/llvm/lib/Support/APInt.cpp:285: int llvm::APInt::compare(const llvm::APInt &) const: Assertion `BitWidth == RHS.BitWidth && "Bit widths must be same for comparison"' failed.
PLEASE submit a bug report to https://github.com/llvm/circt and include the crash backtrace.
Stack dump:
0.      Program arguments: ./build/bin/circt-opt --comb-int-range-narrowing a.mlir
 #0 0x0000000000cdccc7 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /scratch/hidetou/circt/llvm/llvm/lib/Support/Unix/Signals.inc:804:13
 #1 0x0000000000cdac00 llvm::sys::RunSignalHandlers() /scratch/hidetou/circt/llvm/llvm/lib/Support/Signals.cpp:106:18
 #2 0x0000000000cdd5da SignalHandler(int, siginfo_t*, void*) /scratch/hidetou/circt/llvm/llvm/lib/Support/Unix/Signals.inc:0:3
 #3 0x00007ff6960e3cf0 __restore_rt (/lib64/libpthread.so.0+0x12cf0)
 #4 0x00007ff69519cacf raise (/lib64/libc.so.6+0x4eacf)
 #5 0x00007ff69516fea5 abort (/lib64/libc.so.6+0x21ea5)
 #6 0x00007ff69516fd79 _nl_load_domain.cold.0 (/lib64/libc.so.6+0x21d79)
 #7 0x00007ff695195426 (/lib64/libc.so.6+0x47426)
 #8 0x0000000000cf2ee6 (./build/bin/circt-opt+0xcf2ee6)
 #9 0x0000000002dfbf69 llvm::APInt::ult(llvm::APInt const&) const /scratch/hidetou/circt/llvm/llvm/include/llvm/ADT/APInt.h:1111:58
#10 0x0000000002dfbf69 mlir::ConstantIntRanges::rangeUnion(mlir::ConstantIntRanges const&) const /scratch/hidetou/circt/llvm/mlir/lib/Interfaces/InferIntRangeInterface.cpp:93:35
#11 0x0000000000f5907c llvm::APInt::APInt(llvm::APInt&&) /scratch/hidetou/circt/llvm/llvm/include/llvm/ADT/APInt.h:184:39
#12 0x0000000000f5907c mlir::ConstantIntRanges::ConstantIntRanges(mlir::ConstantIntRanges&&) /scratch/hidetou/circt/llvm/llvm/../mlir/include/mlir/Interfaces/InferIntRangeInterface.h:27:7
#13 0x0000000000f5907c std::_Optional_payload_base<mlir::ConstantIntRanges>::_Storage<mlir::ConstantIntRanges, false>::_Storage<mlir::ConstantIntRanges>(std::in_place_t, mlir::ConstantIntRanges&&) /sifive/tools/gcc/gcc/11.2.0/lib/gcc/x86_64-pc-linux-gnu/11.2.0/../../../../include/c++/11.2.0/optional:225:8
#14 0x0000000000f5907c std::_Optional_payload_base<mlir::ConstantIntRanges>::_Optional_payload_base<mlir::ConstantIntRanges>(std::in_place_t, mlir::ConstantIntRanges&&) /sifive/tools/gcc/gcc/11.2.0/lib/gcc/x86_64-pc-linux-gnu/11.2.0/../../../../include/c++/11.2.0/optional:116:4
#15 0x0000000000f5907c std::_Optional_payload<mlir::ConstantIntRanges, true, false, false>::_Optional_payload<mlir::ConstantIntRanges>(std::in_place_t, mlir::ConstantIntRanges&&) /sifive/tools/gcc/gcc/11.2.0/lib/gcc/x86_64-pc-linux-gnu/11.2.0/../../../../include/c++/11.2.0/optional:358:42
#16 0x0000000000f5907c std::_Optional_payload<mlir::ConstantIntRanges, false, false, false>::_Optional_payload<mlir::ConstantIntRanges>(std::in_place_t, mlir::ConstantIntRanges&&) /sifive/tools/gcc/gcc/11.2.0/lib/gcc/x86_64-pc-linux-gnu/11.2.0/../../../../include/c++/11.2.0/optional:392:57
#17 0x0000000000f5907c std::_Optional_base<mlir::ConstantIntRanges, false, false>::_Optional_base<mlir::ConstantIntRanges, false>(std::in_place_t, mlir::ConstantIntRanges&&) /sifive/tools/gcc/gcc/11.2.0/lib/gcc/x86_64-pc-linux-gnu/11.2.0/../../../../include/c++/11.2.0/optional:484:4
#18 0x0000000000f5907c std::optional<mlir::ConstantIntRanges>::optional<mlir::ConstantIntRanges, true>(mlir::ConstantIntRanges&&) /sifive/tools/gcc/gcc/11.2.0/lib/gcc/x86_64-pc-linux-gnu/11.2.0/../../../../include/c++/11.2.0/optional:707:4
#19 0x0000000000f5907c mlir::IntegerValueRange::IntegerValueRange(mlir::ConstantIntRanges) /scratch/hidetou/circt/llvm/llvm/../mlir/include/mlir/Interfaces/InferIntRangeInterface.h:117:48
#20 0x0000000000f5907c mlir::IntegerValueRange::join(mlir::IntegerValueRange const&, mlir::IntegerValueRange const&) /scratch/hidetou/circt/llvm/llvm/../mlir/include/mlir/Interfaces/InferIntRangeInterface.h:145:12
#21 0x0000000000f58f5e llvm::function_ref<void (mlir::Value, mlir::IntegerValueRange const&)>::operator()(mlir::Value, mlir::IntegerValueRange const&) const /scratch/hidetou/circt/llvm/llvm/include/llvm/ADT/STLFunctionalExtras.h:69:12
#22 0x0000000000f58f5e circt::comb::MuxOp::inferResultRangesFromOptional(llvm::ArrayRef<mlir::IntegerValueRange>, llvm::function_ref<void (mlir::Value, mlir::IntegerValueRange const&)>) /scratch/hidetou/circt/lib/Dialect/Comb/InferIntRangeInterfaceImpls.cpp:227:3
#23 0x0000000000f4d103 mlir::detail::InferIntRangeInterfaceInterfaceTraits::Model<circt::comb::MuxOp>::inferResultRangesFromOptional(mlir::detail::InferIntRangeInterfaceInterfaceTraits::Concept const*, mlir::Operation*, llvm::ArrayRef<mlir::IntegerValueRange>, llvm::function_ref<void (mlir::Value, mlir::IntegerValueRange const&)>) /scratch/hidetou/circt/build/tools/mlir/include/mlir/Interfaces/InferIntRangeInterface.h.inc:131:3

uenoku · 2025-04-24T23:42:49Z

lib/Dialect/Comb/Transforms/IntRangeOptimizations.cpp

+    Value lhs = op.getOperand(0);
+    Value rhs = op.getOperand(1);
+


I think this implicitly assumes operations are binary. Add/Mul are variadic so please insert a condition to check op is binary or handle variadic operations as well.
Not sure that's root cause, but I have an example that comb-int-range-narrowing breaks circt-lec. This is basically same logical_ops in test but %7 has 3 operands.

hw.module @logical_ops(in %a : i8, in %b : i9, in %c : i10, in %d : i16, out res : i18) { %c0_i2 = hw.constant 0 : i2 %false = hw.constant false %c0_i9 = hw.constant 0 : i9 %c0_i8 = hw.constant 0 : i8 %0 = comb.concat %c0_i9, %a : i9, i8 %1 = comb.concat %c0_i8, %b : i8, i9 %2 = comb.and %0, %1 : i17 %3 = comb.concat %false, %2 : i1, i17 %4 = comb.concat %c0_i8, %c : i8, i10 %5 = comb.or %3, %4 : i18 %6 = comb.concat %c0_i2, %d : i2, i16 %7 = comb.add %5, %6, %6 : i18 hw.output %7 : i18 }

$ ./build/bin/circt-opt --comb-int-range-narrowing ./test/Dialect/Comb/comb-int-range-narrowing.mlir -o after.mlir $ ./build/bin/circt-lec after.mlir ./test/Dialect/Comb/comb-int-range-narrowing.mlir --c1 logical_ops --c2 logical_ops --shared-libs ../z3/build/libz3.so c1 != c2

Have included a check for binary operators at time of writing - will extend to variadic operators in a subsequent PR, where I'll also add support for narrowing more operators. Good catch!

uenoku · 2025-04-24T23:45:30Z

lib/Dialect/Comb/InferIntRangeInterfaceImpls.cpp

+void comb::ICmpOp::inferResultRanges(ArrayRef<ConstantIntRanges> argRanges,
+                                     SetIntRangeFn setResultRange) {
+  comb::ICmpPredicate combPred = getPredicate();
+  intrange::CmpPredicate pred = static_cast<intrange::CmpPredicate>(combPred);


Could you explicitly convert comb predicate to intrange predicate? There are comb predicates that are specific hardware so this cast is dangerous.

uenoku · 2025-04-25T00:59:30Z

lib/Dialect/Comb/InferIntRangeInterfaceImpls.cpp

+                                        SetIntRangeFn setResultRange) {
+  // Right-shift and truncate (trunaction implicitly handled)
+  auto lowBit = getLowBit();
+  auto umin = argRanges[0].umin().ushl_sat(lowBit);


Yes this is the cause of the InitReg1 bug above - thanks for spotting this error

…owing pass, fix bug in extraction analysis

cowardsa · 2025-04-25T13:34:16Z

@uenoku - have addressed your remaining comments with the following:

Checks for binary operators in Narrowing
Adding support in analysis for variadic ops
Correcting the incorrect shift bug above
Adding tests for cmp, extract and replicate

cowardsa added 10 commits April 9, 2025 10:53

Adding initial template for range analysis pass - currently does not …

13133f4

…implement anything - just returns unknown for every operator

Implement first few basic integer range analysis interpretations for …

683e67d

…comb dialect.

Complete comb interval analysis implementation with the exception of …

3026ee6

…the truthtable and parity operator.

Detecting bits to be removed - not implementing the replacement yet -…

fdc061e

… just return the value of bits that can be removed.

Add pass that reduces every operation to its minimum bitwidth

4cbc064

Format code before publishing branch

3a56e9d

Update with appropriate narrowing named pass - adjust dependencies an…

bac9024

…d add test for new pass.

Add further testing of comb-int-range-narrowing pass

ca34eaa

Update file names and add additional documentation - clean up for loo…

aa0aed4

…p implementations

Minor formatting updates

d980446

cowardsa marked this pull request as ready for review April 17, 2025 11:49

cowardsa requested a review from darthscsi as a code owner April 17, 2025 11:49

TaoBi22 reviewed Apr 17, 2025

View reviewed changes

Intermediate fixes to EOF, working on swapping tests to REGEX and for…

0b36896

…matting corrections

uenoku self-requested a review April 18, 2025 20:20

maerhart reviewed Apr 21, 2025

View reviewed changes

uenoku reviewed Apr 21, 2025

View reviewed changes

lib/Dialect/Comb/InferIntRangeInterfaceImpls.cpp Outdated Show resolved Hide resolved

lib/Dialect/Comb/Transforms/IntRangeOptimizations.cpp Outdated Show resolved Hide resolved

cowardsa added 9 commits April 22, 2025 14:29

Replace variables with regex and minor fixes

34a7d31

Addressing Martin's review comments - mostly coding guideline adherance

79b6a67

Format tidy-up

d230660

Move comb op interfaces to common classes

cec310b

Format operator definitions

22bcbf9

Remove canonicalization pass from int-range-narrowing tests

7cfe57a

Add comb interval range analysis test pass - todo - add a test that a…

7457add

…ctually executes the pass

Add test-comb-int-range-analysis pass and associated tests

a6c3417

Formatting corrections

94696ca

Remove unused includes that broke windows build

4e70663

cowardsa requested review from maerhart and uenoku April 24, 2025 14:01

uenoku approved these changes Apr 25, 2025

View reviewed changes

cowardsa added 4 commits April 25, 2025 11:17

Explicitly cast comb predicates, handle multioperand addition in narr…

607eeab

…owing pass, fix bug in extraction analysis

Extend support to analyze (but not narrow) variadic operators

8aa10ed

Adding tests for replicate, extract and icmp of all variants

688447b

Replace variables in test code with regex

72f6df2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comb Interval Range Analysis and Comb Opt Narrowing pass #8425

Comb Interval Range Analysis and Comb Opt Narrowing pass #8425

cowardsa commented Apr 17, 2025

TaoBi22 left a comment

TaoBi22 Apr 17, 2025

TaoBi22 Apr 17, 2025

cowardsa Apr 18, 2025

cowardsa Apr 18, 2025

maerhart Apr 21, 2025

TaoBi22 Apr 22, 2025

cowardsa Apr 22, 2025

cowardsa Apr 23, 2025

maerhart left a comment

maerhart Apr 21, 2025

uenoku left a comment

cowardsa commented Apr 24, 2025

uenoku left a comment

uenoku Apr 24, 2025

cowardsa Apr 25, 2025

uenoku Apr 24, 2025

uenoku Apr 25, 2025

cowardsa Apr 25, 2025 •

edited

Loading

cowardsa commented Apr 25, 2025

		@@ -0,0 +1,256 @@
		//===- InferIntRangeInterfaceImpls.cpp - Integer range impls for comb -===//

Comb Interval Range Analysis and Comb Opt Narrowing pass #8425

Are you sure you want to change the base?

Comb Interval Range Analysis and Comb Opt Narrowing pass #8425

Conversation

cowardsa commented Apr 17, 2025

TaoBi22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maerhart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uenoku left a comment

Choose a reason for hiding this comment

cowardsa commented Apr 24, 2025

uenoku left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cowardsa Apr 25, 2025 • edited Loading

Choose a reason for hiding this comment

cowardsa commented Apr 25, 2025

cowardsa Apr 25, 2025 •

edited

Loading