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

[krzysz00]

Pinging this PR since I do think the explicit "unknown" state is probably a good idea and I think we all collectively forgot this is here (I found it while cleaning my tabs)

[makslevental]

Pinging this PR since I do think the explicit "unknown" state is probably a good idea and I think we all collectively forgot this is here (I found it while cleaning my tabs)

I have to admit I didn't forget about it but was just procrastinating.

My "best practices" position on this is we should revert #133541 that introduced the ambiguity. As far as I know it is actually no longer necessary for downstream triton-lang/triton#6335. Note, the way in which it's currently "broken" for that particular use case I'm not sure of (@Mogball can comment) but given that it's not currently "load-bearing" we should fix it from "first principles" i.e., Jeff can tell us how it's broken for him currently and we'll try to fix it without the change introduced in #133541. Of course if we fail to find a way to fix without the change, then we can come back and restore the change but also fix the issue discussed in this PR.

[Mogball]

Integer range analysis was miscompiling code without my fix because the way unknown states were being handled wrt control flow was broken. I consider that to be a fairly serious issue. However, it ultimately didn't end up mattering because there were more miscompilation bugs in integer range analysis and I disabled the pass because I didn't have time to track it down. I'm fine with whatever direction here since I don't plan to try integer range analysis anytime soon but just a heads up that there are at least 2 serious bugs with the pass if you revert the fix I landed.

[Mogball]

#133541 has a testcase that demonstrates the issue. You can replace the unregistered ops with function calls, etc. or some inline assembly and you get the same problem.

[krzysz00]

I figured I'd also flag this miscompile #119045