[PATCH] D80860: Exact integer emptiness checks for FlatAffineConstraints

[Arjun P via Phabricator via llvm-commits]

arjunp added a comment.

Changed the `Simplex.cpp` comments to use `///`. Changed `SmallVector` function arguments to `ArrayRef`s.



================
Comment at: mlir/include/mlir/Analysis/Presburger/Matrix.h:66
+  unsigned nColumns;
+  std::vector<std::vector<INT>> data;
+};
----------------
Kayjukh wrote:
> arjunp wrote:
> > Kayjukh wrote:
> > > Why not store the matrix data in a one-dimensional array?
> > This makes the code simpler. In that case, resizing would be more expensive, and resizing is not uncommon, so it's unclear to me if performance considerations weigh in favour of having it be one-dimensional.
> If resizing is a common operation then I agree that it can make sense to keep nested `SmallVector`s. If there is a noticeable performance impact in can always be fixed in a follow-up.
In fact, resizing wouldn't be a problem because we only resize vertically, i.e. we only ever change the number of columns. But another operation that's used frequently is `swapRows` which should also be faster under this representation, so I think we can still keep it this way for now.


================
Comment at: mlir/include/mlir/Analysis/Presburger/Simplex.h:138
+  /// no valid pivot exists.
+  llvm::Optional<std::pair<unsigned, unsigned>>
+  findPivot(int row, Direction direction) const;
----------------
Kayjukh wrote:
> arjunp wrote:
> > Kayjukh wrote:
> > > Could `std::pair` be replaced by a struct with more descriptive member names?
> > Named it `pivotPair`. I wonder if there's a better name.
> `PivotPair`, `PivotPoint`, or even simply `Pivot` all seem reasonable to me.
Named it `Pivot`.


================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:392
+
+void Simplex::undoOp(UndoOp op, llvm::Optional<int> index) {
+  if (op == UndoOp::DEALLOCATE) {
----------------
Kayjukh wrote:
> arjunp wrote:
> > Kayjukh wrote:
> > > This function seems to be a good candidate to be split into two different functions. The branch on the value of `op` seems very unnatural.
> > I could factor out the `Deallocate` branch's body into another function, but I don't understand how the branch can be completely eliminated.
> What I was suggesting was that the function could probably be split into two functions, namely `deallocate` and `unmarkEmpty`. Or is there a reason for doing a dispatch according to the `enum` value?
Yes. When Simplex performs operations, it stores entries in an undo log. When rolling back operations, these log entries are processed and we dispatch the right undo operation depending on the enum value. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80860