This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][Presburger] Implement composition for PresburgerRelation
ClosedPublic

Authored by iambrj on Jul 4 2023, 6:53 AM.

Details

Summary

This patch implements range and domain composition for PresburgerRelations

Diff Detail

Event Timeline

iambrj created this revision.Jul 4 2023, 6:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 4 2023, 6:53 AM
iambrj requested review of this revision.Jul 4 2023, 6:53 AM
Groverkss added a subscriber: grosser.
Groverkss added inline comments.Jul 4 2023, 7:01 AM
mlir/lib/Analysis/Presburger/PresburgerRelation.cpp
113–119

Why do we create a new relation here and not just do it in place?

mlir/unittests/Analysis/Presburger/PresburgerRelationTest.cpp
39

nit: Prefer using -> instead of <->. <-> makes it look like the map is surjective, which may not be true.

Groverkss added inline comments.Jul 4 2023, 7:02 AM
mlir/include/mlir/Analysis/Presburger/PresburgerRelation.h
99–101

Sentences in comments should use full stops at end.

iambrj added inline comments.Jul 4 2023, 10:26 AM
mlir/lib/Analysis/Presburger/PresburgerRelation.cpp
113–119

I considered that but missed that I can change the space using setSpace, so resorted to creating a new relation instead. Will rewrite.

mlir/unittests/Analysis/Presburger/PresburgerRelationTest.cpp
39

I considered ->, but felt -> would be too close to function notation. I wanted to emphasize that these are relations, but I agree <-> makes it look like the map is surjective. Any suggestions for a more relational notation than -> ?

iambrj updated this revision to Diff 537133.Jul 4 2023, 10:54 AM

Address comments

Groverkss accepted this revision.Jul 4 2023, 11:00 AM

LGTM. Just some style comments.

mlir/lib/Analysis/Presburger/PresburgerRelation.cpp
113–115

For a single statement, you can remove braces.

113–119

Ah. Ideally, we should have an inverse in PresburgerSpace as well, which would allow this. I can do that later though.

116–118

Same here, you can remove the braces since its a single statement.

mlir/unittests/Analysis/Presburger/PresburgerRelationTest.cpp
39

Right. MLIR and Presburger already use -> syntax so for consistency sake, it's better to use -> maybe for now.

This revision is now accepted and ready to land.Jul 4 2023, 11:00 AM
iambrj updated this revision to Diff 537135.Jul 4 2023, 11:09 AM
iambrj marked 2 inline comments as done.

Fix style mistakes

iambrj marked 3 inline comments as done.Jul 4 2023, 11:10 AM
This revision was automatically updated to reflect the committed changes.