[PATCH] D80860: Exact integer emptiness checks for FlatAffineConstraints

[Arjun P via Phabricator via llvm-commits]

arjunp added inline comments.


================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:131
+  for (unsigned col = 0; col < nCol; ++col)
+    tableau(row, col) /= gcd;
+}
----------------
grosser wrote:
> arjunp wrote:
> > mehdi_amini wrote:
> > > What guarantees that `gcd` can't be 0 and produce an invalid division here?
> > > Should we guard this loop?
> > > 
> > > (flagged by Coverity)
> > One of the elements being GCD'd is a "denominator" value that is always non-zero. I will add a comment noting this.
> Given that these are coverity errors, it might be useful to document this with an assert to ensure that coverity does not warn about this anymore.
Done: https://reviews.llvm.org/rG0f925339e52e361946a1e894cfa6dbe494b39290


================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:325
+
+    int64_t diff = retConst * elem - constTerm * retElem;
+    if ((diff == 0 && rowUnknown[row] < rowUnknown[*retRow]) ||
----------------
grosser wrote:
> arjunp wrote:
> > mehdi_amini wrote:
> > > mehdi_amini wrote:
> > > > If `retRow` is true, then `retConst` is used uninitialized right? Can you look into this?
> > > > (flagged by Coverity)
> > > (Same with  `retElem`)
> > If `retRow` has a value then lines 319-322 were executed in the iteration that that values was set in, so the values of `retElem` and `retConst` were also set in that iteration already.
> Again, as these are coverity errors the question is how we can avoid coverity to flag these issues. I am almost tempted to just suggest to set retElement and retConst to 0 and add a comment stating that this is done to avoid the coverity warning.
Done: https://reviews.llvm.org/rGef8351598ef33b906fd1962420c8b464812cea85


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80860/new/

https://reviews.llvm.org/D80860