Skip to content

Conversation

@frasercrmck
Copy link
Contributor

@frasercrmck frasercrmck commented Dec 5, 2024

This moves the libspirv libraries to their own subdirectory of libclc, reflecting how the CLC library is organized, and how the OpenCL library may be reorganized in the future.

The libspirv bindings are organized in a similar way to CLC, with a single root directory and then a 'generic' directory containing the target-agnostic implementations, as well as top-level target-specific subdirectories to override specific builtins.

The libspirv bindings still do a lot of the CLC implementations so the separation isn't as clean as it needs to be, but this patch should help to keep our downstream modifications better separated from upstream changes.

Headers that were previously 'spirv' have now been renamed to 'libspirv' to reflect this, and to better distinguish them from the 'spirv' and 'spirv64' libclc targets. That accounts for a majority of the churn.

Using llvm-diff shows no change to either OpenCL/libspirv bitcode library on nvidia targets.

This moves the libspirv libraries to their own subdirectory of libclc,
reflecting how the CLC library is organized, and how the OpenCL library
may be reorganized in the future.

The libspirv bindings are organized in a similar way to CLC, with a
single root directory and then a 'generic' directory containing the
target-agnostic implementations, as well as top-level target-specific
subdirectories to override specific builtins.

The libspirv bindings still do a lot of the CLC implementations so the
separation isn't as clean as it needs to be, but this patch should help
to keep our downstream modifications better separated from upstream
changes.

Headers that were previously 'spirv' have now been renamed to 'libspirv'
to reflect this, and to better distinguish them from the 'spirv' and
'spirv64' libclc targets. That accounts for a majority of the churn.
@frasercrmck frasercrmck requested a review from a team as a code owner December 5, 2024 11:39
@frasercrmck frasercrmck added the libclc libclc project related issues label Dec 5, 2024
@frasercrmck
Copy link
Contributor Author

@sarnex Not sure who to ping, but are you by any chance able to help make sense of what the ProtexIP job is unhappy about?

@sarnex
Copy link
Contributor

sarnex commented Dec 5, 2024

i think it just sucks, let me restart it

@frasercrmck
Copy link
Contributor Author

i think it just sucks, let me restart it

Thanks! It still failed though. I wonder if it's got a problem with some license headers in libclc? I haven't added these files, though - just moved them. I'm not sure what I can do about that - the job output isn't very clear.

@sarnex
Copy link
Contributor

sarnex commented Dec 5, 2024

ok yeah i dont totally understand it either, but if you can say for sure there's no new code in this PR and its just mvoed around, imo we can ignore it

@frasercrmck
Copy link
Contributor Author

ok yeah i dont totally understand it either, but if you can say for sure there's no new code in this PR and its just mvoed around, imo we can ignore it

Oh, I see - it seems that git isn't able to see that I moved rotate.inc to rotate.inc. It's treating it as a new file, which might be tripping this check.

Same with gen_core_convert.py (here -> here).

I think those are the only two it's complaining about?

Speculating, but it could be that the copyright headers are different between our copies of those files, and upstream's. For rotate.inc I see we've added an LLVM Apache License header at some point in the past. For gen_core_convert.py the best I can assume is that upstream has another contributor listed in the header? But then the comparison seems to be against llvm 8, and that contributor wasn't listed back then. I dunno.

Maybe it's best to ignore it - this PR hasn't added anything new in that regard.

@sarnex
Copy link
Contributor

sarnex commented Dec 5, 2024

if youre just moving files i would ignore it

Copy link
Contributor

@jchlanda jchlanda left a comment

Choose a reason for hiding this comment

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

This is a large diff, so I'm assuming that the bulk of the change is coming from libclc/generic/libspirv -> libclc/libspirv/lib/generic layout change and the include adjustment. I like that dir reshuffle, it describes the responsibilities of each component better.

@frasercrmck
Copy link
Contributor Author

@intel/llvm-gatekeepers please merge, thank you. The CI failure is to be ignored (see discussion above)

@steffenlarsen
Copy link
Contributor

Jenkins/Precommit job looks infrastructural. Merging...

@steffenlarsen steffenlarsen merged commit fc44c0b into intel:sycl Dec 9, 2024
13 of 14 checks passed
@frasercrmck frasercrmck deleted the libclc-move-libspirv branch December 9, 2024 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

libclc libclc project related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants