Skip to content

Pass: Do not use llvm::array_pod_sort to sort OpPassManagers. #129968

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

Conversation

pcc
Copy link
Contributor

@pcc pcc commented Mar 6, 2025

OpPassManager contains a field of type std::unique_ptr which
is not guaranteed to be trivially relocatable so we cannot use
llvm::array_pod_sort.

Created using spr 1.3.6-beta.1
@llvmbot llvmbot added mlir:core MLIR Core Infrastructure mlir labels Mar 6, 2025
@pcc pcc requested a review from River707 March 6, 2025 01:45
@llvmbot
Copy link
Member

llvmbot commented Mar 6, 2025

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-core

Author: Peter Collingbourne (pcc)

Changes

OpPassManager contains a field of type std::unique_ptr which
is not guaranteed to be trivially relocatable so we cannot use
llvm::array_pod_sort.


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

1 Files Affected:

  • (modified) mlir/lib/Pass/Pass.cpp (+7-7)
diff --git a/mlir/lib/Pass/Pass.cpp b/mlir/lib/Pass/Pass.cpp
index 6fd51c1e3cb53..67c18189b85e0 100644
--- a/mlir/lib/Pass/Pass.cpp
+++ b/mlir/lib/Pass/Pass.cpp
@@ -678,16 +678,16 @@ LogicalResult OpToOpPassAdaptor::tryMergeInto(MLIRContext *ctx,
   mgrs.clear();
 
   // After coalescing, sort the pass managers within rhs by name.
-  auto compareFn = [](const OpPassManager *lhs, const OpPassManager *rhs) {
+  auto compareFn = [](const OpPassManager &lhs, const OpPassManager &rhs) {
     // Order op-specific pass managers first and op-agnostic pass managers last.
-    if (std::optional<StringRef> lhsName = lhs->getOpName()) {
-      if (std::optional<StringRef> rhsName = rhs->getOpName())
-        return lhsName->compare(*rhsName);
-      return -1; // lhs(op-specific) < rhs(op-agnostic)
+    if (std::optional<StringRef> lhsName = lhs.getOpName()) {
+      if (std::optional<StringRef> rhsName = rhs.getOpName())
+        return *lhsName < *rhsName;
+      return true; // lhs(op-specific) < rhs(op-agnostic)
     }
-    return 1; // lhs(op-agnostic) > rhs(op-specific)
+    return false; // lhs(op-agnostic) > rhs(op-specific)
   };
-  llvm::array_pod_sort(rhs.mgrs.begin(), rhs.mgrs.end(), compareFn);
+  std::sort(rhs.mgrs.begin(), rhs.mgrs.end(), compareFn);
   return success();
 }
 

Copy link
Collaborator

@joker-eph joker-eph left a comment

Choose a reason for hiding this comment

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

There is no C++ trait that allows to check this at compile time?

@pcc
Copy link
Contributor Author

pcc commented Mar 6, 2025

There is no C++ trait that allows to check this at compile time?

There is one but libc++ doesn't use it because it has the wrong semantics:

#if __has_builtin(__is_trivially_relocatable) && 0

@pcc pcc merged commit d58c793 into main Mar 6, 2025
14 checks passed
@pcc pcc deleted the users/pcc/spr/pass-do-not-use-llvmarray_pod_sort-to-sort-oppassmanagers branch March 6, 2025 19:20
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Mar 6, 2025
OpPassManager contains a field of type std::unique_ptr which
is not guaranteed to be trivially relocatable so we cannot use
llvm::array_pod_sort.

Reviewers: River707, joker-eph

Reviewed By: joker-eph

Pull Request: llvm/llvm-project#129968
jph-13 pushed a commit to jph-13/llvm-project that referenced this pull request Mar 21, 2025
OpPassManager contains a field of type std::unique_ptr which
is not guaranteed to be trivially relocatable so we cannot use
llvm::array_pod_sort.

Reviewers: River707, joker-eph

Reviewed By: joker-eph

Pull Request: llvm#129968
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mlir:core MLIR Core Infrastructure mlir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants