This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][Presburger] Remove inheritance from PresburgerSpace in IntegerRelation, PresburgerRelation and PWMAFunction
ClosedPublic

Authored by Groverkss on Apr 12 2022, 3:52 AM.

Details

Summary

This patch removes inheritence from PresburgerSpace in IntegerRelation and
instead makes it a member of these classes.

This is required for three reasons:

  • It prevents implicit casting to PresburgerSpace.
  • Not all functions of PresburgerSpace need to be exposed by the deriving classes.
  • IntegerRelation and IntegerPolyhedron are defined in a PresburgerSpace. It makes more sense for the space to be a member instead of them inheriting from a space.

Diff Detail

Event Timeline

Groverkss created this revision.Apr 12 2022, 3:52 AM
Herald added a project: Restricted Project. · View Herald Transcript
Groverkss requested review of this revision.Apr 12 2022, 3:52 AM
Groverkss added a subscriber: grosser.

"removes inheritence from PresburgerSpace" sounds like PresburgerSpace previously had inheritance but doesn't anymore. Can you clarify this? (both title and summary)

arjunp added inline comments.Apr 12 2022, 4:06 AM
mlir/include/mlir/Analysis/Presburger/IntegerRelation.h
1

why this change?

27–30
597–601

Please rewrite this similarly to suggestion above

mlir/include/mlir/Analysis/Presburger/PWMAFunction.h
146

"should have"

Groverkss retitled this revision from [MLIR][Presburger] Remove inheritence from PresburgerSpace to [MLIR][Presburger] Remove inheritance from PresburgerSpace in IntegerRelation, IntegerPolyhedron and PWMAFunction.Apr 12 2022, 4:18 AM
Groverkss edited the summary of this revision. (Show Details)
Groverkss retitled this revision from [MLIR][Presburger] Remove inheritance from PresburgerSpace in IntegerRelation, IntegerPolyhedron and PWMAFunction to [MLIR][Presburger] Remove inheritance from PresburgerSpace in IntegerRelation, PresburgerRelation and PWMAFunction.
Groverkss updated this revision to Diff 422173.Apr 12 2022, 4:22 AM
Groverkss marked 4 inline comments as done.
  • Address Arjun's comments
mlir/include/mlir/Analysis/Presburger/IntegerRelation.h
1

oops. reverted this back.

arjunp added inline comments.Apr 12 2022, 4:24 AM
mlir/include/mlir/Analysis/Presburger/IntegerRelation.h
599

IntegerPolyhedron

Groverkss updated this revision to Diff 422176.Apr 12 2022, 4:27 AM
Groverkss marked an inline comment as done.
  • Fix typo in IntegerPolyhedron
arjunp accepted this revision.Apr 12 2022, 4:29 AM

Not for this patch, but I think the IntegerPolyhedron documentation needs to be clarified a little more, by mentioning that it's a set rather than a relation like IntegerRelation and giving some examples.

This revision is now accepted and ready to land.Apr 12 2022, 4:29 AM
ftynse requested changes to this revision.Apr 12 2022, 5:00 AM
ftynse added inline comments.
mlir/include/mlir/Analysis/Presburger/IntegerRelation.h
133–142

Nit: please add documentation to these.

219

Does this really have to be virtual? I see that this method was declared virtual in PresburgerSpace, but the only reason for that is to have an extra assertion in IntegerPolyhedron. Adding a vtable to all objects is a huge price to pay for one assertion. The other virtual method is not overriden. Since this class already follows LLVM style isa, it should be easy to do the assert in the parent class with (|| kind != IntegerPolyhedronKind) in addition to the check. If I'm not mistaken, this will devirtualize the entire hierarchy.

This revision now requires changes to proceed.Apr 12 2022, 5:00 AM
arjunp added inline comments.Apr 12 2022, 5:17 AM
mlir/include/mlir/Analysis/Presburger/IntegerRelation.h
219

The current design of MultiAffineFunction overrides these functions, though that needs to be overhauled as well.

Groverkss marked an inline comment as done.Apr 12 2022, 5:18 AM
Groverkss added inline comments.
mlir/include/mlir/Analysis/Presburger/IntegerRelation.h
219

The virtualization is to support FlatAffineValueConstraints::insertId. I don't think it is for IntegerPolyhedron. https://github.com/llvm/llvm-project/blob/main/mlir/lib/Dialect/Affine/Analysis/AffineStructures.cpp#L294

There are two more functions that are virtual which are also virtual to support FlatAffineValueConstraints only. IntegerRelation::removeIdRange and IntegerRelation::clearAndCopyFrom.

ftynse accepted this revision.Apr 12 2022, 8:17 AM
ftynse added inline comments.
mlir/include/mlir/Analysis/Presburger/IntegerRelation.h
219

My bad, haven't seen those. I agree that there needs to be more thought (and experiments) about the overheads of different implementations. Unblocking this patch.

This revision is now accepted and ready to land.Apr 12 2022, 8:17 AM
This revision was landed with ongoing or failed builds.Apr 12 2022, 10:21 AM
This revision was automatically updated to reflect the committed changes.