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

[SingleSource/Atomic] Add preliminary tests for atomic builtins. #78

Closed
wants to merge 0 commits into from

Conversation

Logikable
Copy link

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.

@Logikable Logikable changed the title [SingleSource/Atomic] Add preliminary libatomic tests. [SingleSource/Atomic] Add preliminary tests for atomic builtins. Jan 19, 2024
Copy link
Member

@Meinersbur Meinersbur left a comment

Choose a reason for hiding this comment

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

Have you considered using GTest for these? It's already contained in the llvm-test-suite repository.

SingleSource/UnitTests/Atomic/CMakeLists.txt Outdated Show resolved Hide resolved
SingleSource/UnitTests/Atomic/big_test.cpp Outdated Show resolved Hide resolved
desired.v[k]++;
}
} while (!__atomic_compare_exchange(abig, &expected, &desired, true,
success_model, fail_model));
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Clang emits branches. I'm not sure whether gcc emits branches, but it doesn't at least for int32s on x86.

Copy link
Member

Choose a reason for hiding this comment

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

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.

SingleSource/UnitTests/Atomic/float_test.cpp Outdated Show resolved Hide resolved
@Logikable
Copy link
Author

Updated with suggestions.

Have you considered using GTest for these? It's already contained in the llvm-test-suite repository.

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?

@Meinersbur
Copy link
Member

Meinersbur commented Jan 23, 2024

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?

  • LIT already has infrastructure for GTest
  • cmpxchg/xchg verification should be exact (no floating-point rounding). GTest allows you to later add ASSERT_NEAR tests for atomic add/inc/mul etc without weakening the xchg tests.
  • Fails already at ASSERT_NEAR in the test, instead of fpcmp after the program has already finished.
  • fpcmp is textual comparison, floating-point epsilon-comparision is a workaround on top of it.
  • Test and expected result in the same file

@MaskRay
Copy link
Member

MaskRay commented Jan 30, 2024

@tstellar @zeroomega @pogo59 It seems that llvm-test-suite will need a third-party/unittest/googletest/ as well.

@pogo59
Copy link

pogo59 commented Jan 30, 2024

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.

@zeroomega
Copy link

@tstellar @zeroomega @pogo59 It seems that llvm-test-suite will need a third-party/unittest/googletest/ as well.

Trying to understand the context.
I assume for llvm-test-suite, we want to add/run some unit tests and the conclusion is to use googletest for them?
But llvm-test-suit already has a copy of googletest, located at https://github.com/llvm/llvm-test-suite/tree/7527e72f2c7fc8dc5e1e3548c5ad1e82d4748538/MicroBenchmarks/libs/benchmark/googletest

Maybe move it to a path like third_party/googletest and make it shared by all llvm-test-suite components?

@pogo59
Copy link

pogo59 commented Jan 30, 2024

llvm-test-suit already has a copy of googletest

Aha! Thank you. That is buried pretty deep.

Maybe move it to a path like third_party/googletest and make it shared by all llvm-test-suite components?

SGTM.

@Logikable
Copy link
Author

To add some context to @MaskRay's comment, I have tried and failed to move the currently imported GoogleTest (at Microbenchmarks/libs/benchmark/googletest to a more centralized location so it can be used by any test in this suite.

I have a commit on the way that migrates my tests to GoogleTest, but in a pretty hackish way. Improvement suggestions are welcome.

@Logikable
Copy link
Author

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 Microbenchmarks/libs/benchmark), and despite bodging numerous things to get it to work, it still doesn't. This in addition to the comments above, I think this will be a more significant effort than I can (or should) manage for this PR.

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?

@pogo59
Copy link

pogo59 commented Jan 31, 2024

It might be informative to look at the llvm-project patch to move googletest to a new location:
llvm/llvm-project@a11cd0d

@Logikable
Copy link
Author

Logikable commented Jan 31, 2024

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:

  • When I build it by including its CMake file, its compile time is measured by timeit thanks to https://github.com/llvm/llvm-test-suite/blob/main/CMakeLists.txt#L325-L328. But sometimes, timeit won't have been built by this point. I don't understand CMake or how the lists have been set up remotely enough to know how to fix this race condition. Do I specify a dependency, or do I somehow build GoogleTest before this instrumentation is added?
  • There's a lot of infrastructure to get llvm_singlesource() and llvm_multisource() to work. Ideally, GTest has similar infrastructure, to avoid things like target_link_libraries(${name} GTest::gtest_main) in multiple places. I don't know that I'm the person to do this; it will take me weeks I don't have.

I would be happy to open a separate PR with my patch linked above once GTest has been successfully integrated into this repo.

@pogo59
Copy link

pogo59 commented Jan 31, 2024

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.

@Meinersbur
Copy link
Member

@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 libclang_rt.atomic.so really necessary? Shouldn't the compiler find it itelf? It would be nice if we could keep the test-suite compiler-independent so we can check consistency with other compilers and/or using the system's libatomic -- not everone always builds LLVM with compiler-rt.

@MaskRay
Copy link
Member

MaskRay commented Feb 1, 2024

There is no option to instruct Clang driver to pass libclang_rt.atomic.{a,so} to ld, so --print-file-name=libclang_rt.atomic.so is perhaps the best we can do. I agree that having a way to test libatomic will be really useful. Perhaps add a CMake option?

@arichardson
Copy link
Member

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?

@pogo59
Copy link

pogo59 commented Feb 2, 2024

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?

@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.

@arichardson
Copy link
Member

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?

@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

@Logikable
Copy link
Author

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?

@pogo59
Copy link

pogo59 commented Feb 2, 2024

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.

@arichardson
Copy link
Member

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?

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.

@Logikable
Copy link
Author

Updated with suggestions.

desired.v[k]++;
}
} while (!__atomic_compare_exchange(abig, &expected, &desired, true,
success_model, fail_model));
Copy link
Member

Choose a reason for hiding this comment

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

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.

Comment on lines 26 to 31
// 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.
Copy link
Member

Choose a reason for hiding this comment

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

What is the intent of this? Since it's UB, technically speaking, even executing it could make the entire program execution garbage.

Copy link
Author

Choose a reason for hiding this comment

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

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.

SingleSource/UnitTests/Atomic/big_test.cpp Outdated Show resolved Hide resolved
SingleSource/UnitTests/Atomic/float_test.cpp Outdated Show resolved Hide resolved
@Logikable
Copy link
Author

Updated with suggestions.

@Logikable
Copy link
Author

Updated with suggestions.

@Logikable
Copy link
Author

Gentle bump.

@Logikable
Copy link
Author

Oops. I git reset the wrong directory... remaking this PR now...

@Logikable
Copy link
Author

Created #94.

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.

7 participants