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

[gcc] Prefer bsl/bit/bif for copysignf. (backport GCC-8) #4

Merged
merged 1 commit into from Sep 17, 2023

Conversation

direc85
Copy link
Contributor

@direc85 direc85 commented Aug 31, 2023

This (slightly modified) patch supposedly fixes the compiler segfault @llewelld ran into while updating Gecko from ESR 78 to ESR 91:

47:04.13   cargo:warning=during RTL pass: expand
47:04.13   cargo:warning=src/glsl.h: In function ‘glsl::vec2_scalar
           glsl::sign(glsl::vec2_scalar)’:
47:04.13   cargo:warning=src/glsl.h:662:39: internal compiler error:
           Segmentation fault
47:04.13   cargo:warning= float sign(float a) { return copysignf(1.0f, a); }
47:04.13   cargo:warning=                              ~~~~~~~~~^~~~~~~~~
47:04.13   cargo:warning=Please submit a full bug report,
47:04.13   cargo:warning=with preprocessed source if appropriate.
47:04.14   cargo:warning=See  for instructions.
47:04.14   exit status: 1

I haven't been able to compile nor test this patch in Platform SDK, but with minor tweaks (two changes for Changelog and a one-line add for aarch64.md) the patch applies at least. As such, I wish to someone who is more familiar with this process pick this up and build/test it further.

The original bug I found was: Bug 90075 - [7/8 Regression] [AArch64] ICE during RTL pass when member of union passed to copysignf (also contains code to reproduce the issue - untested my be though).

The unmodified patch can be downloaded here (or from the bug link by following the gcc-8 backport links).

@mlehtima
Copy link
Contributor

You need to run the precheckin.sh script to sync the cross-gcc spec files with gcc.spec.

@Thaodan
Copy link
Contributor

Thaodan commented Sep 13, 2023 via email

This patch is to fix the ICE caused in expand pattern of copysignf
builtin. This is a back port to r267019 of trunk.

gcc:

2019-04-29  Srinath Parvathaneni  <srinath.parvathaneni@arm.com>

	Backport from mainline
	2018-12-11  Richard Earnshaw <Richard.Earnshaw@arm.com>

	PR target/37369
	* config/aarch64/iterators.md (sizem1): Add sizes for
	SFmode and DFmode.
	(Vbtype): Add SFmode mapping.
	* config/aarch64/aarch64.md (copysigndf3, copysignsf3): Delete.
	(copysign<GPF:mode>3): New expand pattern.
	(copysign<GPF:mode>3_insn): New insn pattern.

testsuite:

2019-04-29  Srinath Parvathaneni  <srinath.parvathaneni@arm.com>

	PR target/90075
	* gcc.target/aarch64/pr90075.c: New test.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90075
https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=267019
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=37369
@direc85
Copy link
Contributor Author

direc85 commented Sep 13, 2023

precheckin.sh script ran and commits squashed.

Copy link
Contributor

@Thaodan Thaodan left a comment

Choose a reason for hiding this comment

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

LGTM

@direc85
Copy link
Contributor Author

direc85 commented Sep 13, 2023

We still don't know if this patch actually fixes the problem flypig is facing with gecko build. I would have built this myself, but it looks like not all cross-aarch64 dependencies for GCC are available for i486 built target. The cross-i486 dependencies are available, and I can compile that one, but it doesn't help here. Perhaps OBS helps us out here.

@Thaodan
Copy link
Contributor

Thaodan commented Sep 14, 2023 via email

@rainemak
Copy link
Member

@direc85 this is neat! As discussed earlier today, let's do not merge quite yet.

Quote from @llewelld from IRC meeting:

As it happens the latest changes built for aarch64 on OBS last night, so I can now test this on the gecko source. I don't think it makes sense to merge it before that.

@llewelld
Copy link
Member

I was just able to test this new version, and it does seem to fix the issue I was experiencing. Here's the error output from before:

In file included from src/glsl.h:7,
                 from src/gl.cc:92:
src/vector_type.h: In instantiation of ‘static T glsl::Unaligned<T>::load(const P*) [with P = glsl::VectorType<float, 16>; T = glsl::vec4]’:
src/vector_type.h:532:28:   required from ‘T glsl::unaligned_load(const P*) [with T = glsl::vec4; P = glsl::VectorType<float, 16>]’
src/vector_type.h:543:27:   required from ‘D glsl::bit_cast(const S&) [with D = glsl::vec4; S = glsl::VectorType<float, 16>]’
src/blend.h:53:41:   required from here
src/vector_type.h:503:11: warning: ‘void* memcpy(void*, const void*, size_t)’ writing to an object of type ‘struct glsl::vec4’ with no trivial copy-assignment; use copy-assignment or copy-initialization instead [-Wclass-memaccess]
     memcpy(&v, p, sizeof(v));
     ~~~~~~^~~~~~~~~~~~~~~~~~
In file included from src/gl.cc:92:
src/glsl.h:1796:8: note: ‘struct glsl::vec4’ declared here
 struct vec4 {
        ^~~~
during RTL pass: expand
src/glsl.h: In function ‘glsl::vec2_scalar glsl::sign(glsl::vec2_scalar)’:
src/glsl.h:662:39: internal compiler error: Segmentation fault
 float sign(float a) { return copysignf(1.0f, a); }
                              ~~~~~~~~~^~~~~~~~~
Please submit a full bug report,
with preprocessed source if appropriate.
See <https://git.sailfishos.org/> for instructions.

And this is what I get now (no error!):

In file included from src/glsl.h:7,
                 from src/gl.cc:92:
src/vector_type.h: In instantiation of ‘static T glsl::Unaligned<T>::load(const P*) [with P = glsl::VectorType<float, 16>; T = glsl::vec4]’:
src/vector_type.h:532:28:   required from ‘T glsl::unaligned_load(const P*) [with T = glsl::vec4; P = glsl::VectorType<float, 16>]’
src/vector_type.h:543:27:   required from ‘D glsl::bit_cast(const S&) [with D = glsl::vec4; S = glsl::VectorType<float, 16>]’
src/blend.h:53:41:   required from here
src/vector_type.h:503:11: warning: ‘void* memcpy(void*, const void*, size_t)’ writing to an object of type ‘struct glsl::vec4’ with no trivial copy-assignment; use copy-assignment or copy-initialization instead [-Wclass-memaccess]
     memcpy(&v, p, sizeof(v));
     ~~~~~~^~~~~~~~~~~~~~~~~~
In file included from src/gl.cc:92:
src/glsl.h:1796:8: note: ‘struct glsl::vec4’ declared here
 struct vec4 {
        ^~~~

Great work @direc85!

Copy link
Member

@llewelld llewelld left a comment

Choose a reason for hiding this comment

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

This fixes the issue I was experiencing. Thanks @direc85!

@mlehtima
Copy link
Contributor

I will make a full devel test build with these changs to make sure nothing breaks. If that is ok then I will approve this.

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