Integer range inference has been swapped to the new framework. The integer value range lattices automatically updates the corresponding constant value on update.
Depends on D127173
Paths
| Differential D128866
[mlir] Swap integer range inference to the new framework ClosedPublic Authored by Mogball on Jun 29 2022, 5:09 PM.
Details Summary Integer range inference has been swapped to the new framework. The integer value range lattices automatically updates the corresponding constant value on update. Depends on D127173
Diff Detail
Event TimelineComment Actions So, overall comments - especially since it looks like all the details are basically fine ... wouldn't it still be a good idea to have a wrapper around IntRangeAnalysis that hides the fact that it's a dataflow analysis? Or are we abandoning that wrapper here? Also, how's this meant to interact with SCCP? (Is that future work?) Comment Actions The details can be hidden, but then the analysis would not be composable because... it would be hidden. It can interact with SCCP by just loading both analyses at the same time. This is better than running them separately because the partial results of each analysis can help refine each other. Although there are some imperfections in that interaction with the current setup but you can check https://reviews.llvm.org/D128868 for a fully-functioning composable setup. But this doesn't address River's complaint in your patch of the analysis making two passes over the IR. A monolithic analysis will inherently be more efficient, but that's the price paid for extensibility. At least in this case, they can work together instead of just run separately. Comment Actions LGTM, at least as far as the integer analysis parts go. I'd want to make sure @rriddle's fine with the bigger API surface. This revision is now accepted and ready to land.Jun 30 2022, 8:59 AM Comment Actions It's a bit sad that unnecessary API is being exposed here. I think we'll want to have cleaner wrappers around the lattice state though, feels like a pattern is emerging of grabbing the state -> checking null + uninitialized -> calling getValue.
This revision was landed with ongoing or failed builds.Jul 7 2022, 8:28 PM Closed by commit rGab701975e7f3: [mlir] Swap integer range inference to the new framework (authored by Mogball). · Explain Why This revision was automatically updated to reflect the committed changes. Comment Actions Just as a heads up: I have bisected miscompiles in my downstream compiler to this patch. I have figured out a small reproducer that can be run func.func private @foo() -> index { %0 = arith.constant 10 : index return %0 : index } func.func private @bar(%arg0: index) -> index { %c0 = arith.constant 0 : index %1 = arith.constant 420 : index %7 = arith.cmpi eq, %arg0, %c0 : index cf.cond_br %7, ^bb1(%1 : index), ^bb2 ^bb1(%8: index): // 2 preds: ^bb0, ^bb4 return %8 : index ^bb2: %13 = call @foo() : () -> index cf.br ^bb1(%13 : index) } this will incorrectly fold the return %8 : index to return %c420 : index, despite ^bb2 branching to ^bb1 with a different value as block argument. Tested this on both the current revision: 9ad082eb5a948ffd5d8fc959967d184c3433aca0, the one before this commit and the revision of this commit. Comment Actions I have done some further investigation and the issue isn't 100% with this patch itself but more with the Dataflow analysis itself I believe. The real underlying issue is that: Comment Actions Thanks for finding this! I have a fix in https://reviews.llvm.org/D130829 (please take a look!) If you could copy-paste that into a git issue, it'd be nice too
Revision Contents
Diff 441210 mlir/include/mlir/Analysis/DataFlow/IntegerRangeAnalysis.h
mlir/include/mlir/Analysis/DataFlow/SparseAnalysis.h
mlir/include/mlir/Analysis/DataFlowFramework.h
mlir/include/mlir/Analysis/IntRangeAnalysis.h
mlir/include/mlir/Interfaces/InferIntRangeInterface.td
mlir/lib/Analysis/CMakeLists.txt
mlir/lib/Analysis/DataFlow/DeadCodeAnalysis.cpp
mlir/lib/Analysis/DataFlow/IntegerRangeAnalysis.cpp
mlir/lib/Analysis/DataFlow/SparseAnalysis.cpp
mlir/lib/Analysis/DataFlowFramework.cpp
mlir/lib/Analysis/IntRangeAnalysis.cpp
mlir/lib/Dialect/Arithmetic/Transforms/UnsignedWhenEquivalent.cpp
mlir/lib/Transforms/SCCP.cpp
mlir/test/lib/Analysis/DataFlow/TestDeadCodeAnalysis.cpp
mlir/test/lib/Transforms/TestIntRangeInference.cpp
|
If you want to include unregistered as potential terminators, this should use mightHaveTrait which returns true if the op is unregistered.