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

[libclc] Add initial LIT tests #87989

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

frasercrmck
Copy link
Contributor

@frasercrmck frasercrmck commented Apr 8, 2024

These tests aren't very meaningful and aren't immune to false positives,
but they do get the project building when running 'check-all' and so
enable libclc testing in CI.

Copy link

github-actions bot commented Apr 8, 2024

✅ With the latest revision this PR passed the Python code formatter.

These tests aren't very meaningful and aren't immune to false positives,
but they do get the project building when running 'check-all' and so
enable libclc testing in CI.
@frasercrmck frasercrmck changed the title [WIP] Libclc tests [libclc] Add initial LIT tests Apr 16, 2024
@frasercrmck frasercrmck requested a review from arsenm April 16, 2024 17:16
@@ -1,3 +1,6 @@
__kernel void foo(int *i) {
// RUN: %clang -emit-llvm -S -o - %s | FileCheck %s
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use update_cc_test_checks? How will checks for different targets co-exist? These all need some kind of explicit target to avoid host dependence

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I was hoping to explore in that direction. I've kind of just copied the skeleton of the LIT tests we have downstream, but as you say, this isn't really workable as it assumes all tests will be able to use the same CHECK.

I think, for this to work, we'd have to configure LIT with all of the libclc targets (or, a subset we're interested in), then RUN each test multiple times with a unique FileCheck check prefix for each target. Then when running update_cc_test_checks it'd run the test on all targets.

I think we'd need a macro or substitution to expand a single RUN line to multiple architectures, to keep things maintainable.

We could also allow running the tests on just the one or a subset of architectures locally, using a --param to control the architectures to test.

Does that sound like a good direction? to take this in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ach I misunderstood how update_cc_test_checks works. It doesn't actually go through the regular LIT infrastructure, so we can't do anything involving custom substitutions to configure which target is being run.

If we wanted to be able to use update_cc_test_checks I think we'd need to have every target explicit in every test file, which brings its own inflexibilities and maintenance problems.

Copy link
Contributor

@arsenm arsenm Apr 17, 2024

Choose a reason for hiding this comment

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

The target absolutely should be explicit in any testing files. Pretending these tests can be generic is going to be an intractable problem. This is no different than any clang codegen test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't so much thinking that it would pretend to be generic, but rather that it'd use a custom test format (something like analyzer_test.py) to run the test once for each target we build/test, substituting in the target triple and target-specific check prefix each time.

That way we could test all or a subset of all targets without worrying about the test failing on a target we haven't built.

In that sense these are a little different to clang codegen tests - aren't they? clang can always compile for any of its supported targets. With libclc, however, the user is allowed to build only a handful of the targets, and each of those targets can have drastically different builtin implementations through specialization.

We don't want the tests to fail if the user hasn't built certain targets, so I think we need some kind of configuration that allows tests to be skipped. But update_cc_test_checks doesn't handle features or %if or anything dynamic like that. It's really only suitable for the simplest of clang tests. So if we wanted to have each test file run on multiple targets, I think update_cc_test_checks could only be used if the user built all libclc targets, and each test had one RUN line for each target. I think we have ten targets so it might get a bit unwieldy, is all I'm saying. I do want us to be able to use that script as it's by far the best way of maintaining tests that we've got.

Copy link
Contributor

Choose a reason for hiding this comment

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

This just requires additional REQUIRES support in lit for the built targets when running the test, the same as other built-backend dependent codegen tests. If you're actually updating the tests, you kind of need to have built all the targets

We don't want the tests to fail if the user hasn't built certain targets,

Right, they should be skipped UNSUPPORTED.

In that sense these are a little different to clang codegen tests - aren't they? clang can always compile for any of its supported targets. With libclc, however, the user is allowed to build only a handful of the targets, and each of those targets can have drastically different builtin implementations through specialization.

This is identical to clang codegen tests. Not every builtin emits identical IR, not every builtin is supported on every target, and some clang tests directly emit ISA and do have REQUIRES directives to only run if the backend built. It's something of an accident that clang can always emit target intrinsics without the corresponding backend being built (which in the past were stalled efforts to fix)

Personally I think the build complexity of libclc having a parallel enabled target mechanism is not worth it, and should just be tied to the built backend support for the top level targets. It doesn't take all that long to build for the handful of triples

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, I hadn't really appreciated that update_cc_test_checks was orthogonal to all of these other LIT concepts like REQUIRES and features more broadly, but that's due to my misunderstanding of how that script worked.

Those clang codegen tests that do have REQUIRES are relegated to their own target-specific source files. What we want for libclc I think is ideally to be testing every libclc builtin with every target. I was hoping we could use a single set of test files to avoid duplication, but that doesn't seem possible.

So each libclc target would have their own copy of each test source file: test/amdgcn/smoothstep.cl, test/nvidia/smoothstep.cl, etc? That's not so bad as long as it's easy enough to bring up a new libclc target (maybe with a generator script, though maybe just copy/pasting a directory of tests, removing the old checks, and running update_cc_test_checks is sufficient)? I suppose the amdgpu tests would be amalgamated and would RUN over each sub-target (r600, amdgcn-mesa3d, etc., as appropriate). Similar for NVIDIA and other groups.

I understand the idea of having libclc targets linked to the backend targets. Some, like clspv/clspv64 and spirv/spirv64, aren't intrinsically tied to any specific backend. clspv targets seem to use the "generic" spir/spir64 triples so could probably be unconditionally enabled, and spirv targets are compiled to those same spir/spir64 triples before being compiled to SPIR-V, which we can't test here. My concern is that this might be a breaking change to some downstream users (we don't really know what they're doing with libclc). Either way I'm not sure this needs to concern this initial round of tests - we can probably stomach the complexity.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can have different target coexist in the same test files when appropriate and just multi-list REQUIRES. Clang does this regularly (OpenMP in particular has a lot of multi targeted codegen tests)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true, but I'm still sceptical that we can make the tests simple enough for update_cc_test_checks to be viable. We'd have to embed libclc further into the clang driver for it to work, given that it currently doesn't know where to find libclc libraries for each target, and we can't express the path to libclc libraries in LIT in such a way that update_cc_test_checks would handle it. I'm not convinced clang should necessarily know about libclc. I don't think we should be tying libclc to any specific compilation model like OpenCL C.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is essential that clang should know about libclc. libclc exists purely as an extension of the compiler. From the user perspective the opencl builtin library is invisible and always available, which requires clang to handle it without any additional user configuration

Ideally it should universally be available in the clang resource directory

target = lit_config.params.get("target", "")
builtins = lit_config.params.get("builtins", "")

clang_flags = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally the clang driver would know to do all this with the provided path

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.

None yet

2 participants