[MLIR][Presburger] Add Identifier class to store identifiers and their type
Needs ReviewPublic
Actions

Authored by Groverkss on Mar 26 2023, 5:18 AM.

Details

Reviewers

arjunp
bondhugula
ftynse

Summary

This patch adds a new class Identifier to store identifiers in PresburgerSpace
and their types.

Identifiers were added earlier and were stored as a void pointer, and their type
in the form of mlir::TypeId in PresburgerSpace. To get an identifier, a user of
PresburgerSpace needed to know the type of identifiers. This was a problem for
users of PresburgerSpace like IntegerRelation, which want to work on
identifiers without knowing their type.

The Identifier class allows users like IntegerRelation to work on identifiers
without knowing their type, and also exposes an easier way to work with
Identifiers.

Diff Detail

Repository: rG LLVM Github Monorepo

Event Timeline

Groverkss created this revision.Mar 26 2023, 5:18 AM

Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2023, 5:18 AM

Herald added subscribers: Moerafaat, zero9178, bzcheeseman and 21 others. · View Herald Transcript

Groverkss requested review of this revision.Mar 26 2023, 5:18 AM

Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2023, 5:18 AM

Herald added subscribers: stephenneuendorffer, nicolasvasilache. · View Herald Transcript

Groverkss added a child revision: D146910: [MLIR][Presburger] Fix swap not swapping identifiers in IntegerRelation.Mar 26 2023, 5:27 AM

Groverkss added a child revision: D146912: [MLIR][Analysis] Use Identifiers outside Presburger library.

Harbormaster completed remote builds in B221826: Diff 508403.Mar 26 2023, 5:44 AM

Herald added a subscriber: jsetoain. · View Herald TranscriptMar 26 2023, 5:44 AM

arjunp added inline comments.Fri, May 12, 6:14 AM

mlir/include/mlir/Analysis/Presburger/PresburgerSpace.h
31–35	I think we need much more documentation to explain all this to someone who doesn't already know what it is.
155–159
158	"actually same variable" doesn't seem to make sense within the context of PresburgerSpace. Can you instead say something like, the user can identify each variable in the space as corresponding to some Identifier, and then explain clearly how and where these identifiers are used, if at all. This should make sense when taken together with the identifier documentation
mlir/unittests/Analysis/Presburger/PresburgerSpaceTest.cpp
90–93	why was the API changed here?

Herald added a subscriber: bviyer. · View Herald TranscriptFri, May 12, 6:14 AM

I'd requesting expanding commit summary to include sufficient rationale and summary of the change, and what it enables.

mlir/include/mlir/Analysis/Presburger/PresburgerSpace.h
31–35	I agree. Examples will also help here.
84–86	What's this?

Address comments

Groverkss edited the summary of this revision. (Show Details)Wed, May 17, 6:00 AM

Groverkss added inline comments.

mlir/include/mlir/Analysis/Presburger/PresburgerSpace.h
84–86	oops, removed.
mlir/unittests/Analysis/Presburger/PresburgerSpaceTest.cpp
90–93	To make it consistent with how we do this through Presburger Library. We don't have setters; instead, we use get as a reference and set it that way.

arjunp added inline comments.Wed, May 17, 6:20 AM

mlir/include/mlir/Analysis/Presburger/PresburgerSpace.h
31	or replace with some other examples you have in mind
33	two variables in what? same set? different sets with same space? different space?
39	this notation is unclear. can you write some kind of example statement `S0: A[i][j] = 0` instead
49	The discussion here seems to imply that the names `i` and `j` are somehow relevant here. can your clarify this
56
58
60	not clear what "always unique" means. can you just say it's treated as no attachment and then describe the behaviour of such variables