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

[SPARC] Fix regression from UpgradeDataLayoutString change #110608

Merged
merged 2 commits into from
Oct 2, 2024

Conversation

koachan
Copy link
Contributor

@koachan koachan commented Oct 1, 2024

It turns out that we cannot rely on the presence of -i64:64 as a position reference when adding the -i128:128 datalayout string due to some custom datalayout strings lacking it (e.g ones used by bugpoint, among other things).
Do not add the -i128:128 string in that case.

This fixes the regression introduced in #106951.

It turns out that we cannot rely on the presence of `i64:64` as an "anchor"
when adding the `-i128:128` string due to tools that generate custom
datalayout strings (e.g bugpoint, among other things).
Revert to the generic regex matcher to make sure that we can still add the
i128 string in all other cases.

This fixes the regression introduced in llvm#106951.
@llvmbot
Copy link
Collaborator

llvmbot commented Oct 1, 2024

@llvm/pr-subscribers-llvm-ir

Author: Koakuma (koachan)

Changes

It turns out that we cannot rely on the presence of -i64:64 as a position reference when adding the -i128:128 datalayout string due to some custom datalayout strings lacking it (e.g ones used by bugpoint, among other things).
Revert to the generic regex matcher to make sure that we can still add the i128 string in those cases.

This fixes the regression introduced in #106951.


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

1 Files Affected:

  • (modified) llvm/lib/IR/AutoUpgrade.cpp (+4-4)
diff --git a/llvm/lib/IR/AutoUpgrade.cpp b/llvm/lib/IR/AutoUpgrade.cpp
index 6f833acd6dbc0d..76c8a6db533465 100644
--- a/llvm/lib/IR/AutoUpgrade.cpp
+++ b/llvm/lib/IR/AutoUpgrade.cpp
@@ -5519,12 +5519,12 @@ std::string llvm::UpgradeDataLayoutString(StringRef DL, StringRef TT) {
 
   if (T.isSPARC()) {
     // Add "-i128:128"
-    std::string I64 = "-i64:64";
     std::string I128 = "-i128:128";
     if (!StringRef(Res).contains(I128)) {
-      size_t Pos = Res.find(I64);
-      assert(Pos != size_t(-1) && "no i64 data layout found!");
-      Res.insert(Pos + I64.size(), I128);
+      SmallVector<StringRef, 4> Groups;
+      Regex R("^([Ee](-[mpi][^-]*)*)((-[^mpi][^-]*)*)$");
+      if (R.match(Res, &Groups))
+        Res = (Groups[1] + I128 + Groups[3]).str();
     }
     return Res;
   }

@rorth
Copy link
Collaborator

rorth commented Oct 1, 2024

Confirmed: with this patch, sparcv9-sun-solaris2.11 test results are clean again. Thanks.

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

Wouldn't it be enough to drop the assertion?

If the datalayout is already "incorrect" to the point that it does not have i64 alignment, it probably doesn't make sense to add i128 alignment?

@koachan
Copy link
Contributor Author

koachan commented Oct 2, 2024

Wouldn't it be enough to drop the assertion?

If the datalayout is already "incorrect" to the point that it does not have i64 alignment, it probably doesn't make sense to add i128 alignment?

Hmm, yeah, turning the assert into an if seem to work too. I guess it's better like this.

@koachan koachan requested a review from nikic October 2, 2024 02:27
Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LGTM

@koachan koachan merged commit 076392b into llvm:main Oct 2, 2024
8 checks passed
@koachan koachan deleted the sparc-i128 branch October 2, 2024 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants