Skip to content

[clang] Increase VecLib bitfield size to 4 bits in CodeGenOptions.def #3

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

Closed
wants to merge 2 commits into from

Conversation

MainakSil
Copy link
Owner

Summary:

This PR fixes the issue where the VecLib bitfield in CodeGenOptions.def is too small to accommodate the increasing number of vector libraries. Specifically, the bitfield size was previously set to 3, but with the introduction of more vector libraries (currently 9), the bitfield needed to be expanded to avoid potential issues in vectorization.

In this PR, I have increased the size of the VecLib bitfield from 3 to 4 to account for the additional libraries. This ensures that all 9 vector libraries are correctly encoded and available for use without errors.

Changes Made:
Modified: Increased the VecLib bitfield size from 3 to 4 in clang/include/clang/Basic/CodeGenOptions.def.

Motivation:
This change is necessary to ensure that all vector libraries are properly represented and selectable. The current limitation of the VecLib bitfield size was causing some vectorization opportunities to be lost when more than 3 bits were needed to represent the library options.

Closes:
Fixes llvm#108704

@pawosm-arm
Copy link

For me it looks ok, as it solves the problem at hand, but anyone else would argue that it lacks some protecting mechanism (a test case) that would prevent this situation in the future (as more libraries will be added).

@MainakSil
Copy link
Owner Author

MainakSil commented Sep 16, 2024

Submitted it as PR for the project. Please check and verify

@MainakSil MainakSil closed this Sep 16, 2024
MainakSil pushed a commit that referenced this pull request Sep 16, 2024
When SPARC Asan testing is enabled by PR llvm#107405, many Linux/sparc64
tests just hang like
```
#0  0xf7ae8e90 in syscall () from /usr/lib32/libc.so.6
#1  0x701065e8 in __sanitizer::FutexWait(__sanitizer::atomic_uint32_t*, unsigned int) ()
    at compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp:766
#2  0x70107c90 in Wait ()
    at compiler-rt/lib/sanitizer_common/sanitizer_mutex.cpp:35
#3  0x700f7cac in Lock ()
    at compiler-rt/lib/asan/../sanitizer_common/sanitizer_mutex.h:196
llvm#4  Lock ()
    at compiler-rt/lib/asan/../sanitizer_common/sanitizer_thread_registry.h:98
llvm#5  LockThreads ()
    at compiler-rt/lib/asan/asan_thread.cpp:489
llvm#6  0x700e9c8c in __asan::BeforeFork() ()
    at compiler-rt/lib/asan/asan_posix.cpp:157
llvm#7  0xf7ac83f4 in ?? () from /usr/lib32/libc.so.6
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
```
It turns out that this happens in tests using `internal_fork` (e.g.
invoking `llvm-symbolizer`): unlike most other Linux targets, which use
`clone`, Linux/sparc64 has to use `__fork` instead. While `clone`
doesn't trigger `pthread_atfork` handlers, `__fork` obviously does,
causing the hang.

To avoid this, this patch disables `InstallAtForkHandler` and lets the
ASan tests run to completion.

Tested on `sparc64-unknown-linux-gnu`.
@pawosm-arm
Copy link

Submitted it as PR for the project. Please check and verify

I suspect it would be reverted a day or two after being merged. This is the only static_assert in this file, it will meet resistance. IMO a proper test case would be much safer.

@MainakSil
Copy link
Owner Author

So should I add another file in CMake files to give a test case?

@MainakSil MainakSil deleted the def branch September 16, 2024 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[clang] The VecLib bitfield in CodeGenOptions.def is too small for 9 different vector libraries
2 participants