[mlir] fix IntegerRangeAnalysis::staticallyNonNegative

[makslevental]

After #133541, ConstantIntRanges can sometimes have 0-bitwidth APInt. This leads to

llvm/include/llvm/ADT/APInt.h:1044: bool llvm::APInt::operator[](unsigned int) const: Assertion `bitPosition < getBitWidth() && "Bit position out of bounds!"' failed.

somewhere around here. This isn't a great fix (basically a band-aid) but I'm putting it up so we can centralize discussion of how to actually fix - possibly we might first revert #133541? Not sure.

[llvmbot]

@llvm/pr-subscribers-mlir

Author: Maksim Levental (makslevental)

Changes

After #133541, ConstantIntRanges can sometimes have 0-bitwidth APInt. This leads to

llvm/include/llvm/ADT/APInt.h:1044: bool llvm::APInt::operator[](unsigned int) const: Assertion `bitPosition < getBitWidth() && "Bit position out of bounds!"' failed.

somewhere around here. This isn't a great fix (basically a band-aid) but I'm putting it up so we can centralize discussion of how to actually fix - possibly we might first revert #133541? Not sure.

---

Full diff: https://github.com/llvm/llvm-project/pull/134003.diff

1 Files Affected:

• (modified) mlir/lib/Analysis/DataFlow/IntegerRangeAnalysis.cpp (+3)

diff --git a/mlir/lib/Analysis/DataFlow/IntegerRangeAnalysis.cpp b/mlir/lib/Analysis/DataFlow/IntegerRangeAnalysis.cpp
index c7a950d9a8871..6cbd83c8652e6 100644
--- a/mlir/lib/Analysis/DataFlow/IntegerRangeAnalysis.cpp
+++ b/mlir/lib/Analysis/DataFlow/IntegerRangeAnalysis.cpp
@@ -43,6 +43,9 @@ LogicalResult staticallyNonNegative(DataFlowSolver &solver, Value v) {
   if (!result || result->getValue().isUninitialized())
     return failure();
   const ConstantIntRanges &range = result->getValue().getValue();
+  if (range.umin().getBitWidth() || range.umax().getBitWidth() ||
+      range.smin().getBitWidth() || range.smax().getBitWidth())
+    return false;
   return success(range.smin().isNonNegative());
 }
 

[krzysz00]

Yeah, using zero-bitwidth pairs instead of uninitialized for non-integer values a few PRs back is a choice I'm not entirely sure I agree with, and I'm confused about why it was done. @Mogball

[Mogball]

Uninitialized is different than "I don't know". Using uninitialized was causing correctness errors in the analysis, because ops that depend on both an integer and non integer operand are never having their transfer functions called since the state of the non integer operand is never ready. This gets worse when live code is never marked as live because the operand never gets marked as ready.

[makslevental]

Uninitialized is different than "I don't know".

Agreed but currently we have no model for "I don't know" right? This zero bit width thing is now playing the role but

1. there are no checks upstream for this "I don't know" state

2. it's not a great model since a priori zero bit width isn't synonymous with "I don't know"

3. what's good for the goose is good for the gander: if this reasoning holds (and I think it does), this is actually about all AnalysisStates

[Mogball]

I think it's up to the specific AnalysisState to implement a bottom state. For example, ConstantValue just holds a null Attribute.

it's not a great model since a priori zero bit width isn't synonymous with "I don't know"

I admit this decision was rushed but since I didn't see any tests fail, I assumed no one was actually checking the IntegerRange of a non-integer value. In fact, I think returning any bogus (but initialized) range for a non-integer value is valid.

[makslevental]

So then what do you propose? Just apply the bandaid? I'm fine with that.

[krzysz00]

Ok, so, to clarify: is it required that the bottom state for an analysis be distinct from its unitialized state? Or are they necessarily identical so that an analysis must derive a non-bottom value for any value it is invoked on?

[Mogball]

Ok, so, to clarify: is it required that the bottom state for an analysis be distinct from its unitialized state? Or are they necessarily identical so that an analysis must derive a non-bottom value for any value it is invoked on?

Yes! Sparse data flow analysis treats uninitialized as the top state, i.e. join(uninitialized, x) = x. Therefore, it's a major problem if the bottom state is also uninitialized. E.g. ContantValue is a std::optional<Attribute> and uses std::nullopt to indicate uninitialized, and Attribute() to indicate "unknown value". The problem is that all states are set to uninitialized initially, so if for non-integer values, their pessimistic state is also uninitialized, they will never trigger an update. Also, it wouldn't make any sense.

[krzysz00]

Ah - I had my terminology backwards, then.

I figured that having the non-integer values be std::nullopt would be fine, since the inference method would just get nullopt for its non-integer arguments - that is, for values that aren't integers, the existing code assumed that uninitialized was the only possible inhabitant of the lattice for those types.

Using ConstantIntRanges with 0-bit APInts would be fine as a "we know there can't be data here" value ... but, in order to uphold the interface contract of InferIntRanges, we'll need a followup patch that maps that 0-width stat back to std::nullopt ... and probably need to update anything that consumes integer range lattice values directly to know about this new behavior.

I'm not opposed to adding a distinct end state for non-integer values, but that'll require auditing the code to make sure that that state's being checked correctly.

[Mogball]

I feel like a decent contract for this function would be to assert that isa<IntegerType>(v.getType()), but I don't have a strong opinion here

[krzysz00]

I think the general shake of the solution is to seal all the isUninitialized()s for a new isUnknown() or isNonInteger()

[krzysz00]

... wow that's a lot of autocorrect-os

Hopefully the overall message got through

The other thing that'd be needed there is to make sure that whenever we've visiting a value, we always make the result something other than "unknown"