Skip to content
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

mlir/Presburger: guard dump function; fix buildbot #95218

Merged
merged 1 commit into from
Jun 12, 2024

Conversation

artagnon
Copy link
Contributor

Follow up on 76030dc (mlir/Presburger/MPInt: move into llvm/ADT) to guard a function in Fraction.h with !NDEBUG || LLVM_ENABLE_DUMP, since the call to the corresponding function in DynamicAPInt is guarded similarly. This patch fixes the build when mlir is built with this configuration.

Follow up on 76030dc (mlir/Presburger/MPInt: move into llvm/ADT) to
guard a function in Fraction.h with !NDEBUG || LLVM_ENABLE_DUMP, since
the call to the corresponding function in DynamicAPInt is guarded
similarly. This patch fixes the build when mlir is built with this
configuration.
@llvmbot
Copy link
Collaborator

llvmbot commented Jun 12, 2024

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-presburger

Author: Ramkumar Ramachandra (artagnon)

Changes

Follow up on 76030dc (mlir/Presburger/MPInt: move into llvm/ADT) to guard a function in Fraction.h with !NDEBUG || LLVM_ENABLE_DUMP, since the call to the corresponding function in DynamicAPInt is guarded similarly. This patch fixes the build when mlir is built with this configuration.


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

1 Files Affected:

  • (modified) mlir/include/mlir/Analysis/Presburger/Fraction.h (+13-10)
diff --git a/mlir/include/mlir/Analysis/Presburger/Fraction.h b/mlir/include/mlir/Analysis/Presburger/Fraction.h
index f4f1be97147bf..6be132058e6ca 100644
--- a/mlir/include/mlir/Analysis/Presburger/Fraction.h
+++ b/mlir/include/mlir/Analysis/Presburger/Fraction.h
@@ -52,15 +52,24 @@ struct Fraction {
     return num / den;
   }
 
-  llvm::raw_ostream &print(llvm::raw_ostream &os) const {
-    return os << "(" << num << "/" << den << ")";
-  }
-
   /// The numerator and denominator, respectively. The denominator is always
   /// positive.
   DynamicAPInt num{0}, den{1};
+
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
+  llvm::raw_ostream &print(llvm::raw_ostream &os) const {
+    return os << "(" << num << "/" << den << ")";
+  }
+#endif
 };
 
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
+inline llvm::raw_ostream &operator<<(llvm::raw_ostream &os, const Fraction &x) {
+  x.print(os);
+  return os;
+}
+#endif
+
 /// Three-way comparison between two fractions.
 /// Returns +1, 0, and -1 if the first fraction is greater than, equal to, or
 /// less than the second fraction, respectively.
@@ -156,12 +165,6 @@ inline Fraction &operator*=(Fraction &x, const Fraction &y) {
   x = x * y;
   return x;
 }
-
-inline llvm::raw_ostream &operator<<(llvm::raw_ostream &os, const Fraction &x) {
-  x.print(os);
-  return os;
-}
-
 } // namespace presburger
 } // namespace mlir
 

@artagnon artagnon merged commit 638d968 into llvm:main Jun 12, 2024
7 of 9 checks passed
@artagnon artagnon deleted the presburger-dump-guard branch June 12, 2024 10:00
@akuegel
Copy link
Member

akuegel commented Jun 12, 2024

Unfortunately this is not the only required fix like this. Another example is mlir/include/mlir/Analysis/Presburger/GeneratingFunction.h line 110

And there may be more. Maybe better to revert for now, and wait with rolling forward until you have verified that you have fixed everything?

@artagnon
Copy link
Contributor Author

Unfortunately this is not the only required fix like this. Another example is mlir/include/mlir/Analysis/Presburger/GeneratingFunction.h line 110

And there may be more. Maybe better to revert for now, and wait with rolling forward until you have verified that you have fixed everything?

Thanks for reporting the breakage: I will attempt to fix all the instances. I think a revert wouldn't be wise, as the change is huge, and the only builds that are broken are release builds without LLVM_ENABLE_DUMP.

makslevental added a commit to makslevental/llvm-project that referenced this pull request Jun 12, 2024
@makslevental
Copy link
Contributor

makslevental commented Jun 12, 2024

and the only builds that are broken are release builds without LLVM_ENABLE_DUMP.

Which is probably the majority of builds since it is off by default.

artagnon added a commit to artagnon/llvm-project that referenced this pull request Jun 12, 2024
Fix the build failure arising from 76030dc (mlir/Presburger/MPInt: move
into llvm/ADT) by reverting the failed fix-forward llvm#95218, and removing
the guards on the debug-print functions in DynamicAPInt.
makslevental added a commit that referenced this pull request Jun 12, 2024
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.

None yet

5 participants