[PATCH] D40369: Support sext instruction in SCEV delinearization algorithm (new revision)

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur requested changes to this revision.
Meinersbur added a comment.
This revision now requires changes to proceed.

I applied this patch. However, your test-case fails.



================
Comment at: lib/Analysis/ScalarEvolution.cpp:943
 
+  void treatCastDenominator(const SCEVCastExpr *CENumerator, const SCEV **Q,
+                        const SCEV **R) {
----------------
Please document what this function is supposed to do.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:945-960
+    if (const SCEVTruncateExpr *TruncDenominator =
+                                      dyn_cast<SCEVTruncateExpr>(Denominator)) {
+      divide(SE, CENumerator->getOperand(), TruncDenominator->getOperand(), Q,
+             R);
+    }
+    else
+    if (const SCEVZeroExtendExpr *ZExtDenominator =
----------------
These seem to just ignore casts of the denominator. If `SCEVDivision` implements a signed division, then this should be ok for sext. If it implements unsigned division, it should be ok for zext. However, it is never semantically correct for both at the same time nor for trunc.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:969
+
+    // Note that the division operation here is signed.
+    //  And SExt does not change the value of the divisor nor of the dividend.
----------------
SCEV only has `SCEVUDivExpr`, i.e. cannot represent signed division. Therefore signed division seems the wrong interpretation here.

`SCEVDivision::divide` might actually do a signed division, unfortunately I cannot find any documentation supporting this.


================
Comment at: test/Analysis/Delinearization/test_sext.ll:10
+; CHECK: Base offset: %a
+; CHECK-NEXT: ArrayDecl[UnknownSize][%N][%N] with elements of 4 bytes.
+
----------------
This check fails for me. I get the output:
```
Base offset: %a
ArrayDecl[UnknownSize][%N][%N] with elements of 16 bytes.
```

Maybe you want to check the `ArrayRef` line as well.


Repository:
  rL LLVM

https://reviews.llvm.org/D40369