Add a pass which do arith dialect ops optimization based on integer range analysis (only cmpi for now).
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/include/mlir/Dialect/Arith/Transforms/Passes.td | ||
---|---|---|
52 | Can you merge this pass with ArithUnsignedWhenEquivalentPass? Both of them use the results of integer range inference and it seems that pass can be considered an "integer range optimization" anyways. | |
mlir/lib/Dialect/Arith/Transforms/IntRangeOptimizations.cpp | ||
17–22 | ||
28 | please document this function: what it does and its purpose | |
117 | do you need a pattern for this? Why not just walk the top-level operation of the pass and find all arith.cmpi ops? | |
mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp | ||
1160 ↗ | (On Diff #485114) | Can you add an op to TestOps.td? I think this creates a better separation of concerns with this patch. You can implement the interface on Dialect operations as necessary in subsequent patches. |
mlir/lib/Dialect/Arith/Transforms/IntRangeOptimizations.cpp | ||
---|---|---|
117 | Downstream we are building custom pass which combines integer range analysis with some shape analysis and we need this populateIntRangeOptimizationsPatterns function as a building block, standalone pass needed just to test things in isolation. I can remove dedicated pass and just add these patterns to UnsignedWhenEquivalentPass (although I don't think UnsignedWhenEquivalent is strictly an optimization pass, as backends can have different preferences between signed and unsigned ops) but I need to keep either populateIntRangeOptimizationsPatterns or some other way to express these optimizations separated from pass. |
mlir/lib/Dialect/Arith/Transforms/IntRangeOptimizations.cpp | ||
---|---|---|
117 | Okay, this rationale makes sense. Thanks. You can keep the separate pass |
mlir/lib/Dialect/Arith/Transforms/IntRangeOptimizations.cpp | ||
---|---|---|
149 | please just use a switch statement for this |
mlir/lib/Dialect/Arith/Transforms/IntRangeOptimizations.cpp | ||
---|---|---|
149 | I did it, but it just makes code 2 time bigger and actually less readable |
mlir/include/mlir/Dialect/Arith/Transforms/Passes.h | ||
---|---|---|
20 | You can do this for the new pass gen style | |
mlir/include/mlir/Dialect/Arith/Transforms/Passes.td | ||
59 | Can you omit the constructor and use the new style of pass generation? | |
mlir/lib/Dialect/Arith/Transforms/IntRangeOptimizations.cpp | ||
27–31 | ? | |
34 | please return FailureOr<bool> for these functions instead. Also, just FYI but you can drop the llvm:: on Optional here | |
149 | If you want to compress the code size, you can outline this switch and do case Pred::slt: return handleSlt(lhsValue, rhsValue); case Pred::sle: ... Also, the compiler will turn the switch into a jump table, so I don't think you should be manually creating that. |
mlir/lib/Dialect/Arith/Transforms/IntRangeOptimizations.cpp | ||
---|---|---|
149 | I've tried nested lambda with return but it is even uglier. And I was using table not because I wanted to jump table in asm but because it is a common idiom. switch not worth it, I'm switching back to table. |
Very delayedly, I'd like to point out that a more general version of this code lives out in mlir/test/lib/Transforms/TestIntRangeInference.cpp and thet perhaps we might just want to make that a non-test pass?
Make sense, only requirements from our side is that it must be composable, as we have our custom IntegerRangeAnalysisEx derived from IntegerRangeAnalysis and we adding some additional custom analyses to DataFlowSolver.
Entirely fair - I'm curious about what extensions you've made and if they
can be pulled upstream.
I'd be happy to discuss whether there's something we can unify on here
since it looks like we both have downstream code that uses this analysis.
Here is our pass https://github.com/numba/numba-mlir/blob/main/mlir/lib/Transforms/ShapeIntegerRangePropagation.cpp
We have extended int range analysis for shaped ops (tensor, memref, and our custom ntensor dialects) and propagating int range for each dim independently coupling it with existing analysis on shaped.dim ops.
We were planning to upstream this eventually but don't have much throughput at the moment.
You can do this for the new pass gen style