[llvm] [DA] Fix the check between Subscript and Size after delinearization (PR #151326)

[Ryotaro Kasuga via llvm-commits]

================
@@ -594,14 +594,15 @@ for.end12:                                        ; preds = %for.inc10, %entry
 }
 
 
+; FIXME? It seems that we cannot prove that %N is non-negative...
 define void @nonnegative(ptr nocapture %A, i32 %N) {
 ; CHECK-LABEL: 'nonnegative'
 ; CHECK-NEXT:  Src: store i32 1, ptr %arrayidx, align 4 --> Dst: store i32 1, ptr %arrayidx, align 4
-; CHECK-NEXT:    da analyze - none!
+; CHECK-NEXT:    da analyze - output [* *]!
 ; CHECK-NEXT:  Src: store i32 1, ptr %arrayidx, align 4 --> Dst: store i32 2, ptr %arrayidx, align 4
-; CHECK-NEXT:    da analyze - consistent output [0 0|<]!
+; CHECK-NEXT:    da analyze - output [* *|<]!
 ; CHECK-NEXT:  Src: store i32 2, ptr %arrayidx, align 4 --> Dst: store i32 2, ptr %arrayidx, align 4
-; CHECK-NEXT:    da analyze - none!
+; CHECK-NEXT:    da analyze - output [* *]!
----------------
kasuga-fj wrote:

> I seems doubious to me that < is implemented in terms of getMinusSCEV < 0. The more I think about it the more worried I become. The issue can easily be shown when using constants.

I couldn't agree more. I believe this patch fixes some bugs, but not sure if this is a foundamental solution. And probably the similar bugs exist in other places of DA as well...

With my earlier comment, "all values should be interpreted as signed", what I meant was that we should proceed with the analysis only if we can prove that none of the subscripts overflow in the signed sense. If we encounter a value that can overflow (e.g., transitioning from `0b01111111` to `0b10000000`), or if we cannot prove that this doesn't happen, we should bail out early and return something like "unknown", in DA, probably a `Dependence` object instead of a `FullDependence` one. It may also be fine to assume that all values are unsigned, but anyway, it would be safer to unify the interpretation used in subsequent processing. Mixing signed and unsigned representations complicates things and, in my opinion, will lead to inconsistencies with the mathematical foundations of the DA algorithm.

Also, I think we need to pay more attention to the SCEV computation (like `getAddExpr`) within each function (ZIV, SIV, MIV, etc.). At the very least, the calculation on `SCEVConstant` seems to wrap silently (in two-complement sense) if they are sufficently large. Although it might sound a bit extreme, I'm starting to think we should replace all `ScalaEvolution::getAddExpr` invocations in DA with something like `getAddExprIfNoConstantOverflow` (Minus and Mul as well). Once an overflow is detected, the analysis should be terminated immediately. Or, using wider types in such cases would be another solution.

There is one bit of good news; regarding an AddRec used as an argument of GEP, maybe we can reason about wrapping behavior it if the GEP has `nusw` flag (not the wraps about the during the GEP operation, the AddRec itself!). Looking at [this commit message](https://github.com/llvm/llvm-project/commit/a353e25), the same logic seems to apply in DA as well. However, at the same time, I also have the following concerns:

- It says that the AddRec in the GEP argument doesn't wrap. So, what about subscripts after delinearization? At the moment, I think it would also not wrap if sizes are also positive (signed sense? unsigned sense?).
- The result of the GEP being poison itself doesn't trigger UB. It will be triggered only when it is used as the pointer argument to a load/store. So, we might need to add some control-flow checks, to rule out cases like this:

  ```llvm
    ...
    %gep = getelementptr inbounds i32, ptr %A, i64 %addrec
    %cond = ... ; an expression that always evaluates to false whenever %gep is poison
    br i1 %cond, label %if.then, label %sink

  if.then:
    store i32 42, ptr %gep
    br %sink

  sink:
    ...
  ```

https://github.com/llvm/llvm-project/pull/151326