Skip to content

Implement insertVar having Identifier parameter #76993

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zjaffal
Copy link
Contributor

@zjaffal zjaffal commented Jan 4, 2024

No description provided.

@llvmbot
Copy link
Member

llvmbot commented Jan 4, 2024

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-presburger

Author: Zain Jaffal (zjaffal)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/76993.diff

8 Files Affected:

  • (modified) mlir/include/mlir/Analysis/FlatLinearValueConstraints.h (+3-2)
  • (modified) mlir/include/mlir/Analysis/Presburger/IntegerRelation.h (+4-2)
  • (modified) mlir/include/mlir/Analysis/Presburger/PresburgerRelation.h (+2-1)
  • (modified) mlir/include/mlir/Analysis/Presburger/PresburgerSpace.h (+2-4)
  • (modified) mlir/lib/Analysis/FlatLinearValueConstraints.cpp (+3-2)
  • (modified) mlir/lib/Analysis/Presburger/IntegerRelation.cpp (+6-5)
  • (modified) mlir/lib/Analysis/Presburger/PresburgerRelation.cpp (+3-3)
  • (modified) mlir/lib/Analysis/Presburger/PresburgerSpace.cpp (+4-3)
diff --git a/mlir/include/mlir/Analysis/FlatLinearValueConstraints.h b/mlir/include/mlir/Analysis/FlatLinearValueConstraints.h
index e4de5b0661571c..2458ef3004b8f2 100644
--- a/mlir/include/mlir/Analysis/FlatLinearValueConstraints.h
+++ b/mlir/include/mlir/Analysis/FlatLinearValueConstraints.h
@@ -346,8 +346,9 @@ class FlatLinearValueConstraints : public FlatLinearConstraints {
   unsigned insertSymbolVar(unsigned pos, ValueRange vals);
   using FlatLinearConstraints::insertSymbolVar;
 
-  unsigned insertVar(presburger::VarKind kind, unsigned pos,
-                     unsigned num = 1) override;
+  unsigned
+  insertVar(presburger::VarKind kind, unsigned pos, unsigned num = 1,
+            presburger::Identifier id = presburger::Identifier()) override;
   unsigned insertVar(presburger::VarKind kind, unsigned pos, ValueRange vals);
 
   /// Removes variables in the column range [varStart, varLimit), and copies any
diff --git a/mlir/include/mlir/Analysis/Presburger/IntegerRelation.h b/mlir/include/mlir/Analysis/Presburger/IntegerRelation.h
index 4c6b810f92e95a..aa96dfb3eaf99b 100644
--- a/mlir/include/mlir/Analysis/Presburger/IntegerRelation.h
+++ b/mlir/include/mlir/Analysis/Presburger/IntegerRelation.h
@@ -275,7 +275,8 @@ class IntegerRelation {
   /// corresponding to the added variables are initialized to zero. Return the
   /// absolute column position (i.e., not relative to the kind of variable)
   /// of the first added variable.
-  virtual unsigned insertVar(VarKind kind, unsigned pos, unsigned num = 1);
+  virtual unsigned insertVar(VarKind kind, unsigned pos, unsigned num = 1,
+                             Identifier id = Identifier());
 
   /// Append `num` variables of the specified kind after the last variable
   /// of that kind. The coefficient columns corresponding to the added variables
@@ -895,7 +896,8 @@ class IntegerPolyhedron : public IntegerRelation {
   /// Positions are relative to the kind of variable. Return the absolute
   /// column position (i.e., not relative to the kind of variable) of the
   /// first added variable.
-  unsigned insertVar(VarKind kind, unsigned pos, unsigned num = 1) override;
+  unsigned insertVar(VarKind kind, unsigned pos, unsigned num = 1,
+                     Identifier id = Identifier()) override;
 
   /// Return the intersection of the two relations.
   /// If there are locals, they will be merged.
diff --git a/mlir/include/mlir/Analysis/Presburger/PresburgerRelation.h b/mlir/include/mlir/Analysis/Presburger/PresburgerRelation.h
index c6b00eca90733a..e21f37ed867d8b 100644
--- a/mlir/include/mlir/Analysis/Presburger/PresburgerRelation.h
+++ b/mlir/include/mlir/Analysis/Presburger/PresburgerRelation.h
@@ -64,7 +64,8 @@ class PresburgerRelation {
   /// exceeds that of some disjunct, an assert failure will occur.
   void setSpace(const PresburgerSpace &oSpace);
 
-  void insertVarInPlace(VarKind kind, unsigned pos, unsigned num = 1);
+  void insertVarInPlace(VarKind kind, unsigned pos, unsigned num = 1,
+                        Identifier id = Identifier());
 
   /// Converts variables of the specified kind in the column range [srcPos,
   /// srcPos + num) to variables of the specified kind at position dstPos. The
diff --git a/mlir/include/mlir/Analysis/Presburger/PresburgerSpace.h b/mlir/include/mlir/Analysis/Presburger/PresburgerSpace.h
index 9fe2abafd36bad..0a9daed8762ef3 100644
--- a/mlir/include/mlir/Analysis/Presburger/PresburgerSpace.h
+++ b/mlir/include/mlir/Analysis/Presburger/PresburgerSpace.h
@@ -215,10 +215,8 @@ class PresburgerSpace {
   /// Positions are relative to the kind of variable. Return the absolute
   /// column position (i.e., not relative to the kind of variable) of the
   /// first added variable.
-  ///
-  /// If identifiers are being used, the newly added variables have no
-  /// identifiers.
-  unsigned insertVar(VarKind kind, unsigned pos, unsigned num = 1);
+  unsigned insertVar(VarKind kind, unsigned pos, unsigned num = 1,
+                     Identifier id = Identifier());
 
   /// Removes variables of the specified kind in the column range [varStart,
   /// varLimit). The range is relative to the kind of variable.
diff --git a/mlir/lib/Analysis/FlatLinearValueConstraints.cpp b/mlir/lib/Analysis/FlatLinearValueConstraints.cpp
index 69846a356e0cc4..b1a81dd83ddfd0 100644
--- a/mlir/lib/Analysis/FlatLinearValueConstraints.cpp
+++ b/mlir/lib/Analysis/FlatLinearValueConstraints.cpp
@@ -9,6 +9,7 @@
 #include "mlir/Analysis//FlatLinearValueConstraints.h"
 
 #include "mlir/Analysis/Presburger/LinearTransform.h"
+#include "mlir/Analysis/Presburger/PresburgerSpace.h"
 #include "mlir/Analysis/Presburger/Simplex.h"
 #include "mlir/Analysis/Presburger/Utils.h"
 #include "mlir/IR/AffineExprVisitor.h"
@@ -870,8 +871,8 @@ unsigned FlatLinearValueConstraints::insertSymbolVar(unsigned pos,
 }
 
 unsigned FlatLinearValueConstraints::insertVar(VarKind kind, unsigned pos,
-                                               unsigned num) {
-  unsigned absolutePos = IntegerPolyhedron::insertVar(kind, pos, num);
+                                               unsigned num, Identifier id) {
+  unsigned absolutePos = IntegerPolyhedron::insertVar(kind, pos, num, id);
 
   if (kind != VarKind::Local) {
     values.insert(values.begin() + absolutePos, num, std::nullopt);
diff --git a/mlir/lib/Analysis/Presburger/IntegerRelation.cpp b/mlir/lib/Analysis/Presburger/IntegerRelation.cpp
index 0109384f1689dd..28b368ece33afe 100644
--- a/mlir/lib/Analysis/Presburger/IntegerRelation.cpp
+++ b/mlir/lib/Analysis/Presburger/IntegerRelation.cpp
@@ -334,10 +334,11 @@ IntegerRelation::subtract(const PresburgerRelation &set) const {
   return PresburgerRelation(*this).subtract(set);
 }
 
-unsigned IntegerRelation::insertVar(VarKind kind, unsigned pos, unsigned num) {
+unsigned IntegerRelation::insertVar(VarKind kind, unsigned pos, unsigned num,
+                                    Identifier id) {
   assert(pos <= getNumVarKind(kind));
 
-  unsigned insertPos = space.insertVar(kind, pos, num);
+  unsigned insertPos = space.insertVar(kind, pos, num, id);
   inequalities.insertColumns(insertPos, num);
   equalities.insertColumns(insertPos, num);
   return insertPos;
@@ -2478,11 +2479,11 @@ void IntegerRelation::print(raw_ostream &os) const {
 
 void IntegerRelation::dump() const { print(llvm::errs()); }
 
-unsigned IntegerPolyhedron::insertVar(VarKind kind, unsigned pos,
-                                      unsigned num) {
+unsigned IntegerPolyhedron::insertVar(VarKind kind, unsigned pos, unsigned num,
+                                      Identifier id) {
   assert((kind != VarKind::Domain || num == 0) &&
          "Domain has to be zero in a set");
-  return IntegerRelation::insertVar(kind, pos, num);
+  return IntegerRelation::insertVar(kind, pos, num, id);
 }
 IntegerPolyhedron
 IntegerPolyhedron::intersect(const IntegerPolyhedron &other) const {
diff --git a/mlir/lib/Analysis/Presburger/PresburgerRelation.cpp b/mlir/lib/Analysis/Presburger/PresburgerRelation.cpp
index 787fc1c659a12e..d04dd9eecb5f60 100644
--- a/mlir/lib/Analysis/Presburger/PresburgerRelation.cpp
+++ b/mlir/lib/Analysis/Presburger/PresburgerRelation.cpp
@@ -41,10 +41,10 @@ void PresburgerRelation::setSpace(const PresburgerSpace &oSpace) {
 }
 
 void PresburgerRelation::insertVarInPlace(VarKind kind, unsigned pos,
-                                          unsigned num) {
+                                          unsigned num, Identifier id) {
   for (IntegerRelation &cs : disjuncts)
-    cs.insertVar(kind, pos, num);
-  space.insertVar(kind, pos, num);
+    cs.insertVar(kind, pos, num, id);
+  space.insertVar(kind, pos, num, id);
 }
 
 void PresburgerRelation::convertVarKind(VarKind srcKind, unsigned srcPos,
diff --git a/mlir/lib/Analysis/Presburger/PresburgerSpace.cpp b/mlir/lib/Analysis/Presburger/PresburgerSpace.cpp
index cf1b3befbc89f8..946d53a28a400d 100644
--- a/mlir/lib/Analysis/Presburger/PresburgerSpace.cpp
+++ b/mlir/lib/Analysis/Presburger/PresburgerSpace.cpp
@@ -108,7 +108,8 @@ VarKind PresburgerSpace::getVarKindAt(unsigned pos) const {
   llvm_unreachable("`pos` should represent a valid var position");
 }
 
-unsigned PresburgerSpace::insertVar(VarKind kind, unsigned pos, unsigned num) {
+unsigned PresburgerSpace::insertVar(VarKind kind, unsigned pos, unsigned num,
+                                    Identifier id) {
   assert(pos <= getNumVarKind(kind));
 
   unsigned absolutePos = getVarKindOffset(kind) + pos;
@@ -122,10 +123,10 @@ unsigned PresburgerSpace::insertVar(VarKind kind, unsigned pos, unsigned num) {
   else
     numLocals += num;
 
-  // Insert NULL identifiers if `usingIds` and variables inserted are
+  // Insert id identifiers if `usingIds` and variables inserted are
   // not locals.
   if (usingIds && kind != VarKind::Local)
-    identifiers.insert(identifiers.begin() + absolutePos, num, Identifier());
+    identifiers.insert(identifiers.begin() + absolutePos, num, id);
 
   return absolutePos;
 }

@joker-eph
Copy link
Collaborator

Can provide a description to the PR?
Also I don't see any test change right now?

Copy link
Member

@Groverkss Groverkss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the patch! Help is very welcome :) I have two requests:

  1. Could you please add some unit tests? Presburger lib unit tests live in mlir/unittests/Analysis/Presburger. You should be able to find some Identifier tests there.
  2. Could we keep this patch restricted to Presburger lib for now? I mentioned the exact place where we shouldn't expose it for now as a comment with more explanation.

Comment on lines 873 to +875
unsigned FlatLinearValueConstraints::insertVar(VarKind kind, unsigned pos,
unsigned num) {
unsigned absolutePos = IntegerPolyhedron::insertVar(kind, pos, num);
unsigned num, Identifier id) {
unsigned absolutePos = IntegerPolyhedron::insertVar(kind, pos, num, id);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we restrict this change only to the Presburger library for now? We want to make other things use Identifiers as well, and are currently changing things to make them work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we restrict this change only to the Presburger library for now?

So I will need to remove modifications for insertVar in IntegerPolyhedron, FlatLinearValueConstraints and PresburgerRelation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants