[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.
@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.
Actually, I see this error because I'm running clang tests w/o building SYCL project. When SYCL project is built, the SYCL header are being copied to the build/include directory, where clang is able to find them.
Another problem is that this test runs clang-linker-wrapper without --dry-run option, so the tool calls spirv-to-ir-wrapper, llvm-spirv and sycl-post-link. The problem is these tools are set as dependencies for the check-clang target.
Considering all these problems with the test, I suggest we move it from the driver to SYCL runtime project, where we can use a simple command to build the input for clang-linker-wrapper and all dependencies are satisfied.
There was a problem hiding this comment.
I'm afraid I don't know. I've had little exposure to any of the linker or packager interfaces so far. My familiarity is mostly limited to the front end and basic clang driver interfaces.
Doesn't upstream use compiler built-ins instead of integration header? I thought I can use built-ins the test code and pass some options to the driver to avoid using integration header.
If we can't avoid using integration header, it's fine.
There was a problem hiding this comment.
Upstream will use such builtins, but they are still in development. It will be a bit before we have a fully functional interface for the SYCL library to use.
There was a problem hiding this comment.
@bader For precommit build we use a Docker image with the nightly preinstalled. If you have an idea on how to improve CI to catch this issue please let us know.
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.
Replaced 3 commands calling the front-end compiler and the offload-packager to build a fat object with one call of the driver.