Page MenuHomePhabricator

[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 Timeline

Mogball created this revision.Jun 29 2022, 5:09 PM
Herald added a project: Restricted Project. · View Herald Transcript
Mogball requested review of this revision.Jun 29 2022, 5:09 PM

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?)

Mogball added a comment.EditedJun 29 2022, 11:24 PM

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.

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.

krzysz00 accepted this revision.Jun 30 2022, 8:59 AM
This revision is now accepted and ready to land.Jun 30 2022, 8:59 AM
rriddle accepted this revision.Jul 6 2022, 2:40 AM

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.

mlir/lib/Analysis/DataFlow/DeadCodeAnalysis.cpp
173–175

If you want to include unregistered as potential terminators, this should use mightHaveTrait which returns true if the op is unregistered.

Mogball added inline comments.Jul 7 2022, 2:11 PM
mlir/lib/Analysis/DataFlow/DeadCodeAnalysis.cpp
173–175

That would give a false positive for unregistered operations that aren't the last operation in the block. Visiting these operations would be a no-op in the analysis, but that's extra computation that can be avoided.

Mogball updated this revision to Diff 443126.Jul 7 2022, 8:26 PM

change isTerminator to be more precise

This revision was landed with ongoing or failed builds.Jul 7 2022, 8:28 PM
This revision was automatically updated to reflect the committed changes.

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
via mlir-opt %s --pass-pipeline="func.func(sccp)":

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.

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.
Specifically, the reason that this patch causes this miscompile seems to be because of the lines removed at DataFlowFramework.cpp:90 which served as a sort of mitigataion I believe.

The real underlying issue is that:
https://github.com/llvm/llvm-project/blob/ab701975e7f3b63bb474afbdeb8c474950d41074/mlir/lib/Analysis/DataFlow/SparseAnalysis.cpp#L109
queries for a predecessor state of a call that is never created/written to by DCE. Since PredecessorsState optimistically assumes all predecessors are know, and also has no known predecessors at the same time, it does nothing but return at line 117.
I can only guess, but it seems to me the intention was that this predecessor state should be set in either:
https://github.com/llvm/llvm-project/blob/ab701975e7f3b63bb474afbdeb8c474950d41074/mlir/lib/Analysis/DataFlow/DeadCodeAnalysis.cpp#L410
or
https://github.com/llvm/llvm-project/blob/ab701975e7f3b63bb474afbdeb8c474950d41074/mlir/lib/Analysis/DataFlow/DeadCodeAnalysis.cpp#L300
or maybe during priming of the analysis.
The former does not work as the analysis is scheduled on func.func operations, hence it never seeing the func.return of @foo.
The latter only sets the call as predecessor to the callable.
I think what is needed is to mark the predecessors of the call as unknown if the callable is not part of the analysis.

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