This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][Presburger] Refactor MultiAffineFunction to be defined over universe
ClosedPublic

Authored by Groverkss on Aug 14 2022, 10:47 AM.

Details

Summary

This patch refactors MAF to be defined over the universe in a given space
instead of being defined over a restricted domain.

The reasoning for this refactor is to store division representation for local
variables explicitly for the function outputs. This change is required for
unionLexMax/Min to support local variables which will be upstreamed after this
patch. Another reason for this refactor is to have a flattened form of
AffineMap as MultiAffineFunction.

Diff Detail

Event Timeline

Groverkss created this revision.Aug 14 2022, 10:47 AM
Groverkss requested review of this revision.Aug 14 2022, 10:47 AM
arjunp added inline comments.Aug 20 2022, 9:14 AM
mlir/include/mlir/Analysis/Presburger/PWMAFunction.h
25–26
26
61
67
81–84

Can you use a different name from restrict, e.g., domain

217
mlir/include/mlir/Analysis/Presburger/Utils.h
161–162

Can you please add a doc?

mlir/lib/Analysis/Presburger/PWMAFunction.cpp
42

I don't see why you need to rewrite this function. Just use divValuesAt instead of containsPointNoLocal here

Also, it should probably be getDivValuesAt

118

You can use llvm::all_of here

130–148
131

why is this function much bigger now?

Can you add functions to DivRepr to make these kinds of things easier for users of DivRepr?

187–188

I prefer this for symmetry with other.pieces

187–193

Can rewrite this with llvm::all_of

218
272–273

Why don't we check if the function has compatible spaces? This ensures that the range is compatible as well.

Also please fix the message

272–273

Also can you add a Piece::isCompatible() { return domain.getSpace().isCompatible(func.getDomainSpace() } and check it in various places?

366–371

Can you factor this out into another function?

Mayeb a constructor for IntegerRelation

378

You can use .assign

382–384

Please write some documentation here. Also, use getOffset whenever possible

386

Use getOffset?

mlir/lib/Analysis/Presburger/Simplex.cpp
470–476

This seems annoying. Please add an overload for addPiece and keep the code here the same as it was before.

mlir/unittests/Analysis/Presburger/Utils.h
85–90

Same as above, add an overload to keep this the same it was before. Externally we shouldn't have to be messing with this stuff

Groverkss updated this revision to Diff 459066.Sep 9 2022, 7:56 AM
Groverkss marked 22 inline comments as done.

Address Arjun's comments

mlir/lib/Analysis/Presburger/PWMAFunction.cpp
131

I plan to reduce the size of this as a later patch which will require refactoring the current division merging approach. Doing it now would lead to a lot of unrelated changes.

272–273

Added as isConsistent.

386

I don't think you can use getOffset here.

mlir/lib/Analysis/Presburger/Simplex.cpp
470–476

Simplified this, but I'm against putting an overload in addPiece for this. It does not make sense with the interface of PWMAFunction and MultiAffineFunction.

mlir/unittests/Analysis/Presburger/Utils.h
85–90

I plan to remove this function in this next patch in favour of a better function anyway where this messing with stuff should not be required.

Write better docs for MutliAffineFunction::getAsRelation

arjunp accepted this revision.Sep 10 2022, 2:35 PM
arjunp added inline comments.
mlir/lib/Analysis/Presburger/PWMAFunction.cpp
386

I think you have now used it at the place where this comment was, thanks

This revision is now accepted and ready to land.Sep 10 2022, 2:35 PM