Skip to content

[AArch64] - cannot build from release/18.x #101358

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

catull
Copy link

@catull catull commented Jul 31, 2024

Ran into the same problem reported in the mailing list at https://www.mail-archive.com/[email protected]/msg73342.html.

The PR fixes the build.

Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot llvmbot added the libc label Jul 31, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 31, 2024

@llvm/pr-subscribers-libc

Author: catull (catull)

Changes

Ran into the same problem reported in the mailing list at https://www.mail-archive.com/llvm-bugs@lists.llvm.org/msg73342.html.

The PR fixes the build.


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

1 Files Affected:

  • (modified) libc/src/__support/FPUtil/aarch64/fenv_darwin_impl.h (+6-4)
diff --git a/libc/src/__support/FPUtil/aarch64/fenv_darwin_impl.h b/libc/src/__support/FPUtil/aarch64/fenv_darwin_impl.h
index ea1fd68a5fcdf..efd956f14a8a2 100644
--- a/libc/src/__support/FPUtil/aarch64/fenv_darwin_impl.h
+++ b/libc/src/__support/FPUtil/aarch64/fenv_darwin_impl.h
@@ -161,8 +161,8 @@ LIBC_INLINE int set_except(int excepts) {
 LIBC_INLINE int raise_except(int excepts) {
   float zero = 0.0f;
   float one = 1.0f;
-  float large_value = FPBits<float>::max_normal();
-  float small_value = FPBits<float>::min_normal();
+  float large_value = FPBits<float>::max_normal().get_val();
+  float small_value = FPBits<float>::min_normal().get_val();
   auto divfunc = [](float a, float b) {
     __asm__ __volatile__("ldr  s0, %0\n\t"
                          "ldr  s1, %1\n\t"
@@ -277,8 +277,10 @@ LIBC_INLINE int set_env(const fenv_t *envp) {
     return 0;
   }
   const FEnv::FPState *state = reinterpret_cast<const FEnv::FPState *>(envp);
-  FEnv::set_control_word(state->ControlWord);
-  FEnv::set_status_word(state->StatusWord);
+  uint32_t control_word = static_cast<uint32_t>(state->ControlWord);
+  uint32_t status_word = static_cast<uint32_t>(state->StatusWord);
+  FEnv::set_control_word(control_word);
+  FEnv::set_status_word(status_word);
   return 0;
 }
 

@overmighty
Copy link
Member

We could just cherry-pick commit be8b2d1 instead.

@overmighty
Copy link
Member

I'm not familiar with backporting fixes to release branches. I think LLVM has some special infrastructure for this where we can make a bot account open a PR for us.

@tschuett tschuett requested a review from tru July 31, 2024 17:13
@catull
Copy link
Author

catull commented Jul 31, 2024

@overmighty I am not so sure @MosheBerman's fix does it.

You need both values for large_value and small_value further down, this is line 164 and 165.

As for the other change, don't you need to preserve the control and status words, in line 280 / 281 ?

@overmighty
Copy link
Member

The only difference I see between your PR and be8b2d1 is that you moved the static_casts into new variables instead of having them inside the FEnv function calls directly. It should be functionally identical.

@catull
Copy link
Author

catull commented Jul 31, 2024

The only difference I see between your PR and be8b2d1 is that you moved the static_casts into new variables instead of having them inside the FEnv function calls directly. It should be functionally identical.

I misinterpreted the diff; you are right.
My next commit closes the gap.

@tru
Copy link
Collaborator

tru commented Aug 1, 2024

Hi! 18.x release is done and we don't accept any additional patches for that branch. Please test if this works as expected on LLVM 19.x

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.

5 participants