Skip to content

Conversation

pvchupin
Copy link
Contributor

@pvchupin pvchupin commented May 13, 2022

Can be used to enable all supported llvm compiler targets in the compiler build,
currently:

  • NVPTX
  • AMDGPU

Compiler backends do not require any 3rd party software to build and
test, this can be useful in some local testing, for example when
bringing changes from upstream

Also minor fixes to docs based on currently supported switches.

This is NFC for all existing processes.

Can be used to enable all supported backends in the compiler build,
currently:
* NVPTX
* AMDGPU
Compiler backends do not require any 3rd party software to build and
test, this can be useful in some local testing, for example when
bringing changes from upstream

Also minor fixes to docs based on currently supported switches.

This is NFC for all existing processes.
@pvchupin pvchupin requested review from a team as code owners May 13, 2022 23:17
@pvchupin pvchupin changed the title [SYCL][Build] Add --enable-all-backends switch to configure.py [SYCL][Build] Add --enable-all-llvm-targets switch to configure.py May 17, 2022
@pvchupin pvchupin requested a review from bader May 17, 2022 21:29
@bader bader added cuda CUDA back-end hip Issues related to execution on HIP backend. labels May 18, 2022
Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

The change looks good to me, but I think libclc impacts compilation as well. Is it okay to skip libclc build?

@pvchupin
Copy link
Contributor Author

The change looks good to me, but I think libclc impacts compilation as well. Is it okay to skip libclc build?

As far as I remember libclc has some dependency on 3rd party low-level runtimes, so it stays out of scope of this option, and included only when specific target is fully enabled in build (through --hip or --cuda switches).

@pvchupin
Copy link
Contributor Author

I was wrong. It doesn't have any dependency it seems. At least build and testing is successful.
There is currently 1 LIT fail on AMDGPU target, libclc presence/absence in the build is not affecting that.

I also realized that after adding libclc there is going to be significant (but not a complete) overlap with --ci-defaults option, which seems actually is trying to accomplish the same idea, build everything without deps. The remaining differences in --ci-defaults:

  • warnings by default
  • compiler-rt and clang-tools-extra presence
  • no AMDGPU llvm target, that's a gap I think. I'll submit a patch to address that separately... but LIT issue above should be fixed first.

On the other hand, and actually to answer @bader question, it seems libclc not required for NVPTX and AMDGPU (build and testing). So probably we should limit this change/switch to only that, to keep things limited to compiler targets (as new option says).

Tagging @AerialMantis, @npmiller for opinion

@bader
Copy link
Contributor

bader commented May 19, 2022

On the other hand, and actually to answer @bader question, it seems libclc not required for NVPTX and AMDGPU (build and testing). So probably we should limit this change/switch to only that, to keep things limited to compiler targets (as new option says).

Building libclc is not required, but if a test program calls built-in functions implemented in libclc, having pre-built libclc improves compiler testing coverage. So, it really depends on what is our goal here. I assume it's to test as much as possible w/o external SW and HW dependencies. Am I right?

@pvchupin
Copy link
Contributor Author

I assume it's to test as much as possible w/o external SW and HW dependencies. Am I right?

@bader, I started with this as a primary goal, then realized that --ci-defaults is sort of doing the same thing already.
Now I'm thinking this patch as is makes more sense, as it is only enables all compiler targets and corresponding LLVM BE LIT tests.
LLVM BE LIT tests don't require libclc, right?

If no other suggestions I'm leaning towards landing it in current state.

@npmiller
Copy link
Contributor

I'm not sure how useful it will be in practice, as compiler changes should probably be tested against SYCL lit tests as well, but if you think there's a use case for it, I'm happy with the flag as it is as it's clear it only enables the compiler targets.

@pvchupin pvchupin merged commit 5e6642a into intel:sycl May 23, 2022
@pvchupin pvchupin deleted the build-scripts branch May 23, 2022 23:20
yinyangsx pushed a commit to yinyangsx/llvm that referenced this pull request May 25, 2022
…ntel#6153)

Can be used to enable all supported backends in the compiler build,
currently:
* NVPTX
* AMDGPU
Compiler backends do not require any 3rd party software to build and
test, this can be useful in some local testing, for example when
bringing changes from upstream

Also minor fixes to docs based on currently supported switches.

This is NFC for all existing processes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cuda CUDA back-end hip Issues related to execution on HIP backend.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants