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

Add -mbmi flag to Abseil for GCC versions greater than 14. #4638

Merged
merged 2 commits into from
Apr 30, 2024
Merged

Conversation

fruffy
Copy link
Collaborator

@fruffy fruffy commented Apr 25, 2024

No description provided.

@vlstill
Copy link
Contributor

vlstill commented Apr 25, 2024

This is quite weird. The BMI (bit manipulation instructions) seems to have been introduced in Intel Haswell. While that is 11 years old now, there are definitely computers running older HW still present around. I would expect GCC to be able lower the builtin to something else without the -mbmi. If we compiler with -mbmi we risk users hitting invalid instruction.

@vlstill
Copy link
Contributor

vlstill commented Apr 25, 2024

This is quite confusing. There is a GCC issue saying this builtin (__builtin_ctzs) will not be added: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112789. Yet apparently the compiler used on fedore somehow reports it has it, even when it does not actually: https://github.com/abseil/abseil-cpp/blob/master/absl/numeric/internal/bits.h#L306C23-L306C37.

@vlstill
Copy link
Contributor

vlstill commented Apr 25, 2024

Also, GCC 14 is not yet released officially. I don't think we should be testing with it. Are we using a released version of Fedora?

@vlstill
Copy link
Contributor

vlstill commented Apr 25, 2024

Yep, fedora 40 is shipping unreleased GCC version. Too bad abseil does not have any way to override the check.

@ChrisDodd
Copy link
Contributor

ChrisDodd commented Apr 25, 2024

Not clear why only fedora gives this error -- the default target for gcc on most machines is generic x86_64 (ie, no additional extensions over what was in the original), so it would not be using or requiring bmi instructions before. Has something changed with abseil such that it does thing with shorts that weren't being done before? We use __builtin_ctz in our bitvec code, but only for int/long/long long, not for short

@fruffy
Copy link
Collaborator Author

fruffy commented Apr 25, 2024

This is quite weird. The BMI (bit manipulation instructions) seems to have been introduced in Intel Haswell. While that is 11 years old now, there are definitely computers running older HW still present around. I would expect GCC to be able lower the builtin to something else without the -mbmi. If we compiler with -mbmi we risk users hitting invalid instruction.

It could be the CI infrastructure changing. I can add a flag that this is only added with GCC >14 and some checks. First, I need to get this to work at all.

@fruffy fruffy added run-ubuntu18 Use this tag to trigger a Ubuntu-18 CI run. run-sanitizer Use this tag to run a Clang+Sanitzers CI run. run-static Use this tag to trigger static build CI run. labels Apr 25, 2024
@fruffy fruffy force-pushed the fruffy/fedora branch 2 times, most recently from 4170c5d to 2c18b8b Compare April 27, 2024 19:53
@fruffy fruffy changed the title Add mbmi flag to fix Fedora compilation problems. Add -march=native flag to Abseil for GCC versions greater than 14. Apr 27, 2024
@fruffy fruffy marked this pull request as ready for review April 27, 2024 20:20
@fruffy
Copy link
Collaborator Author

fruffy commented Apr 27, 2024

I do not understand how changing the flags for Abseil can cause -werror to be set for ubpf program compilation. I guess -Wno-error was set previously?

@fruffy fruffy force-pushed the fruffy/fedora branch 2 times, most recently from 3c4151c to 40c7e7d Compare April 29, 2024 15:14
@fruffy
Copy link
Collaborator Author

fruffy commented Apr 29, 2024

This PR is ready now. The fix to the failing uBPF programs is in #4644.

@fruffy fruffy requested review from vlstill and asl April 29, 2024 15:15
@asl
Copy link
Contributor

asl commented Apr 29, 2024

@fruffy This smells like a mainline bug. Not sure where, actually. But maybe start with Abseil?

@fruffy
Copy link
Collaborator Author

fruffy commented Apr 29, 2024

@fruffy This smells like a mainline bug. Not sure where, actually. But maybe start with Abseil?

abseil/abseil-cpp#1664 but unclear whether things will be resolved there.

@asl
Copy link
Contributor

asl commented Apr 29, 2024

@fruffy This smells like a mainline bug. Not sure where, actually. But maybe start with Abseil?

abseil/abseil-cpp#1664 but unclear whether things will be resolved there.

Thanks! Yes, let's see what is there. IIRC -march=native was not quite compatible with arm64/clang, but it's only for GCC, so I'd guess we are fine for now.

@@ -53,7 +53,10 @@ macro(p4c_obtain_abseil)
# Do not suppress warnings for Abseil library targets that are aliased.
get_target_property(target_type ${target} TYPE)
if (NOT ${target_type} STREQUAL "INTERFACE_LIBRARY")
set_target_properties(${target} PROPERTIES COMPILE_FLAGS "-Wno-error -w")
if (CMAKE_COMPILER_IS_GNUCC AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 14)
target_compile_options(${target} PUBLIC "-march=native")
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a cmake warning there? This could produce surprise incompatibilities in the binary. And it might not be that far in future before someone tries to build a release binary with GCC 14.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also it might be good to reference the abseil issue in a comment here. Hopefully we will be able to remove this hack in future.

@vlstill
Copy link
Contributor

vlstill commented Apr 30, 2024

#4643 make me think about this a bit more. I suggest enabling just the minimal set of flags (i.e. -mbmi) for the GCC 14 case. We should still try to preserve as much compatibility as we can.

@fruffy fruffy changed the title Add -march=native flag to Abseil for GCC versions greater than 14. Add -mbmi flag to Abseil for GCC versions greater than 14. Apr 30, 2024
Copy link
Contributor

@vlstill vlstill left a comment

Choose a reason for hiding this comment

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

Looks good to me now. I just suggest a minor extension of the warning.

cmake/Abseil.cmake Outdated Show resolved Hide resolved
Co-authored-by: Vladimír Štill <[email protected]>
@fruffy fruffy enabled auto-merge April 30, 2024 12:35
@fruffy fruffy added this pull request to the merge queue Apr 30, 2024
Merged via the queue into main with commit 6efece7 Apr 30, 2024
17 of 18 checks passed
@fruffy fruffy deleted the fruffy/fedora branch April 30, 2024 14:08
@asl asl mentioned this pull request Jun 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-sanitizer Use this tag to run a Clang+Sanitzers CI run. run-static Use this tag to trigger static build CI run. run-ubuntu18 Use this tag to trigger a Ubuntu-18 CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants