This paves the way for integer-exact projection, and for supporting
non-division locals in subtraction, complement, and equality checks.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| mlir/lib/Analysis/Presburger/IntegerRelation.cpp | ||
|---|---|---|
| 167–175 | I think a better way to write this loop would be to write it in reverse. Swapping each non-div with the offset + numDivisions and incrementing numDivisions after each iteration. | |
| 167–175 | Also, please write some comments explaining what this does. I find this somewhat non-trivial to understand. | |
| 1172–1174 | Not for this patch, but I do not like that we compute the division representation each time we want it. This computation is very expensive. We should instead be storing these divisions separately with each set. | |
| mlir/lib/Analysis/Presburger/IntegerRelation.cpp | ||
|---|---|---|
| 167–175 | numDivisions does not get incremented on every iteration in that implementation. i is only incremented in some cases as well, same as here. Why are you saying that way is better? | |
| mlir/lib/Analysis/Presburger/IntegerRelation.cpp | ||
|---|---|---|
| 167–175 | Sorry for being unclear. I was just suggesting that I think it is more natural to write this loop in reverse. I find that loops working with divisions are easier to write when written in reverse. But since you are not using the division representation, this should be either way. | |
Fix bug where the resulting set had a space incompatible with the space of the original set.
| mlir/include/mlir/Analysis/Presburger/IntegerRelation.h | ||
|---|---|---|
| 101–104 | This is very confusing and I cannot understand what this does. Can you please write it more clearly? | |
| 747–749 | I think writing about union of convex disjuncts is enough. | |
| mlir/include/mlir/Analysis/Presburger/PresburgerRelation.h | ||
| 60 | I would rather call them "extra" ids. | |
| 63 | ||
| mlir/unittests/Analysis/Presburger/PresburgerSetTest.cpp | ||
| 764 | I think you should put a newline after this. | |
This is very confusing and I cannot understand what this does. Can you please write it more clearly?