[SingleSource/Atomic] Add preliminary tests for atomic builtins. #78
Conversation
Logikable
changed the title
| desired.v[k]++; | ||
| } | ||
| } while (!__atomic_compare_exchange(abig, &expected, &desired, true, | ||
| success_model, fail_model)); |
There was a problem hiding this comment.
I am a bit surprised __atomic_compare_exchange allows non-constant for success_model and fail_model since these may map to different instructions. Does Clang/gcc emit branches for them? Or does it possibly pessimize to the lowest common denominator in which case this would test just sequential consistency (even if atomic_compare_exchange_models contains other values)? Could be resolved by making them template arguments.
There was a problem hiding this comment.
Clang emits branches. I'm not sure whether gcc emits branches, but it doesn't at least for int32s on x86.
There was a problem hiding this comment.
Yes; this is OK now for Clang, but it would be better to use constant values for the model. That avoids the risk of a future codesize optimization deciding to collapse it all into 'seq_cst', and silently lose the coverage you wanted here.
|
Updated with suggestions.
The first draft of these tests was written with GTest, but I dropped it along the way. What are some reasons switching to GTest is worth the investment? |
|
|
@tstellar @zeroomega @pogo59 It seems that llvm-test-suite will need a
|
|
Trying to understand where these tests are best placed. @Meinersbur llvm-project has a copy of GTest but afaict llvm-test-suite does not. Am I just not finding it? Or are you suggesting that these tests should be moved to llvm-project? Given that the test appears to be looking at interactions between libraries, probably cross-project-tests would be the most appropriate place for them, within llvm-project. OTOH, if the tests take a significant amount of time to run, llvm-test-suite is probably a better home for them. |
Trying to understand the context. Maybe move it to a path like |
Aha! Thank you. That is buried pretty deep.
SGTM. |
|
To add some context to @MaskRay's comment, I have tried and failed to move the currently imported GoogleTest (at I have a commit on the way that migrates my tests to GoogleTest, but in a pretty hackish way. Improvement suggestions are welcome. |
|
I've put in the better part of a week into setting up the infrastructure for GoogleTest to work across the repository (instead of just within You can find my attempt at https://github.com/Logikable/llvm-test-suite/tree/bodge. @Meinersbur can you provide a code example of what I can do to hook things up? |
|
It might be informative to look at the llvm-project patch to move googletest to a new location: |
|
Thanks for sharing that patch. I want to be more firm about pushing back against me moving these tests to GTest for now. Getting GTest to work requires much more than moving it to a new location:
I would be happy to open a separate PR with my patch linked above once GTest has been successfully integrated into this repo. |
|
It looks like you've made a real effort, and I can sympathize with the difficulties of recoding a bunch of CMake stuff. At this point I'd think the best way forward is to go ahead with the non-GTest based tests, and file an issue to get gtest moved. |
|
@Logikable My original question was just whether you considered it. Your code looked too GTest-like to just ignore it. However, I see your reasons and would be ok to move forward without it. May put a TODO in the source, for instance where FP_TOLERANCE is being worked around. One more concern though: is explicit specifying the path to |
|
There is no option to instruct Clang driver to pass |
|
For the tests that explicitly link against libclang_rt.atomic.so, would it make more sense to have them as tests in the compiler-rt directory? IIRC there are already some basic libatomic tests in that directory, so this would be a natural place to add more? |
@arichardson if you're referring to the llvm-project/compiler-rt directory, the point is that those are not executable tests. This one would actually be executed. |
The ones in compiler-rt are in fact executable not just textual codegen comparisons. While they are slightly more awkward to run than a simple native CMake binary, I have run those tests in past for various architectures while improving the softfloat code. There is at least one libatomic test that I recall touching in the past: compiler-rt/test/builtins/Unit/atomic_test.c |
|
AFAICT, my tests are more expensive than the existing ones in atomic_test.c, taking O(minutes) to run. I'm not familiar with the test suite in llvm-project -- given this, is it okay to move them there? |
|
One of the purposes of llvm-test-suite is to be a place to put longer-running tests. I would not put multiple-minute tests in llvm-project. |
Ah I did not realize the tests would take that long to run. I agree that tests in llvm-project should not take long to run and something well above 1 minute seems problematic (I locally set the lit timeout to 120s per test to catch very slow running tests). Slow running tests have been quite annoying in the past when trying to run them on QEMU (e.g. the libc++ threading tests), so keeping them external makes sense. We could of course reduce the number of iterations but that reduces the probability of catching a race. |
|
Updated with suggestions. |
| desired.v[k]++; | ||
| } | ||
| } while (!__atomic_compare_exchange(abig, &expected, &desired, true, | ||
| success_model, fail_model)); |
There was a problem hiding this comment.
Yes; this is OK now for Clang, but it would be better to use constant values for the model. That avoids the risk of a future codesize optimization deciding to collapse it all into 'seq_cst', and silently lose the coverage you wanted here.
| // Each test also tests the corresponding nonatomic operation with a separate | ||
| // shared variable. Ideally, this value differs from the atomic "correct" value, | ||
| // and the test can compare the two. In reality, some simpler operations (e.g. | ||
| // those conducted through the ALU) can still end up with the correct answer, | ||
| // even when performed nonatomically. These tests do not check the nonatomic | ||
| // result, although it is still outputted to aid in debugging. |
There was a problem hiding this comment.
What is the intent of this? Since it's UB, technically speaking, even executing it could make the entire program execution garbage.
There was a problem hiding this comment.
Oops, this comment is not accurate anymore. We no longer output its results. Additionally, some of the tests do check the nonatomic result, which will likely cause flakiness down the line. I'll remove those checks.
The nonatomic version verifies that the test is meaningful in testing atomicity -- a test may appear to test atomicity, but pass even with nonatomic operations.
I feel the best way to move forward is to remove the nonatomic versions, and leave a note to future test writers to check that their test fails with nonatomic operations.
|
Updated with suggestions. |
|
Updated with suggestions. |
|
Gentle bump. |
|
Oops. I git reset the wrong directory... remaking this PR now... |
|
Created #94. |
There exist atomic IR unit tests and libatomic unit tests, but neither can test the atomicity and interoperability of atomic builtins and compiler-rt's atomic library. These tests aim to approximate behaviour encountered in user code.
These tests have caught issues in Clang. See
llvm/llvm-project#74349 and llvm/llvm-project#73176 for LLVM changes inspired by these tests.