[NFC][Driver] Simplify clang-linker-wrapper test. #17560
Conversation
Replaced 3 commands calling front-end and offload-packager for building fat object with one call of the driver.
| // RUN: %clang -cc1 -fsycl-is-device -disable-llvm-passes -triple=spir64-unknown-unknown %s -emit-llvm-bc -o %t.device.bc | ||
| // RUN: clang-offload-packager -o %t.fat --image=file=%t.device.bc,kind=sycl,triple=spir64-unknown-unknown | ||
| // RUN: %clang -cc1 %s -triple=x86_64-unknown-linux-gnu -emit-obj -o %t.o -fembed-offload-object=%t.fat | ||
| // RUN: %clang -fsycl -fsycl-targets=spir64-unknown-unknown -c --offload-new-driver -Xclang -disable-llvm-passes %s -o %t.o |
There was a problem hiding this comment.
I'm not sure what is the purpose of -Xclang -disable-llvm-passes. Can we drop it?
Should I set the host triple? I suppose in our CI driver will always pass -triple=x86_64-unknown-linux-gnu to the front-end compiler, but on outside of our CI auto-detect might pass different triple.
There was a problem hiding this comment.
The command line for building the device library neither pass -Xclang -disable-llvm-passes nor tests the host triple.
%clang %t.devicelib.cpp -fsycl -fsycl-targets=spir64-unknown-unknown -c --offload-new-driver -o %t.devicelib.o
There was a problem hiding this comment.
I'm not sure what is the purpose of -Xclang -disable-llvm-passes
-Xclang <arg>, will pass <arg> to clang -cc1.
In this case, will pass -disable-llvm-passes to -cc1.
But from Clang help , -disable-llvm-passes is used together with -emit-llvm to get pristine LLVM IR from the frontend by not running any LLVM passes at all.
These options may not be needed for this test case.
There was a problem hiding this comment.
I don't think the host triple is required here, as the test is for linker-wrapper behaviors, which should only really care about what is in the packager. If we were to add the host triple, we should also add the proper REQUIRES
There was a problem hiding this comment.
I re-build my local workspace and now I see following error:
In file included from <built-in>:1:
/tmp/lit-tmp-3lhdupqw/sycl-linker-wrapper-image-header-711582.h:3:10: fatal error: 'sycl/detail/defines_elementary.hpp' file not found
3 | #include <sycl/detail/defines_elementary.hpp>
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
Which seem to be correct. Device compiler emits an integration header, which includes headers from the SYCL runtime. Driver tests should not rely on SYCL runtime project. I guess that was the reason for building commands manually. Unfortunately, no one has written a note about that.
Unless you have better ideas, I'm going to revert most of my changes back and leave only small clean-ups. I'll add a comment about reasons to avoid calling the clang driver for building the test.
I'm concerned that pre-commit testing didn't catch this issue.
@intel/dpcpp-devops-reviewers, any thoughts on that? Do we use docker image with pre-installed SYCL compiler? This could explain how clang managed to find SYCL headers - they are probably in the system paths.
There was a problem hiding this comment.
@bader For precommit build we use a Docker image with the nightly preinstalled.
As I explained in this comment, it's not related to the installed compiler, but rather to the order of actions performed by pre-commit job.
If you have an idea on how to improve CI to catch this issue please let us know.
Please, remove "Compile" step from the pre-commit. It should catch the problem.
There was a problem hiding this comment.
There will be some usability impact, because then we will both be doing compile and test in the check parts of the build and they will take a lot longer. I could add a check-target only job to the nightly or postcommit, but I think for precommit since people will be monitoring the results we should make what is happening easy to understand.
interested in other opinions from @intel/dpcpp-devops-reviewers
There was a problem hiding this comment.
There will be some usability impact, because then we will both be doing compile and test in the
checkparts of the build and they will take a lot longer.
I disagree. They will take same time or most likely be faster than existing "Compile" + "check". There is no value in doing separate "Compile" step before doing "check". Moreover, there are side effects of building "sycl" target before testing LLVM or Clang projects.
There was a problem hiding this comment.
I meant more about people looking at the results than the time it would take, but I don't have a strong opinion. Let's see what the other devops people say.
There was a problem hiding this comment.
FWIW, llvm-project CI pre-commit does build and testing using single ninja command invocation. They have one step to infer which targets we need to build to test modifications done by PR. The second step is to build them at all once. Usually, just a few of check-* targets with -k 0 option to get as much errors as possible.
Replaced 3 commands calling the front-end compiler and the offload-packager to build a fat object with one call of the driver.