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.

Sign up for GitHub

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

[NFC][Driver] Simplify clang-linker-wrapper test. #17560

Closed
wants to merge 1 commit into from
Closed

[NFC][Driver] Simplify clang-linker-wrapper test. #17560

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions clang/test/Driver/sycl-linker-wrapper-image.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,7 @@
//
// Generate .o file as linker wrapper input.
//
// 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
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'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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

//
// Generate .o file as SYCL device library file.
//
Expand All @@ -14,7 +12,7 @@
//
// Run clang-linker-wrapper test
//
//// RUN: clang-linker-wrapper --print-wrapped-module --host-triple=x86_64-unknown-linux-gnu \
// RUN: clang-linker-wrapper --print-wrapped-module --host-triple=x86_64-unknown-linux-gnu \
// RUN: -sycl-device-libraries=%t.devicelib.o \
// RUN: -sycl-post-link-options="-split=auto -symbols -properties" %t.o -o %t.out 2>&1 --linker-path="/usr/bin/ld" | FileCheck %s

Expand Down Expand Up @@ -53,12 +51,12 @@ int main() {
// CHECK-DAG: @SYCL_PropSetName.3 = internal unnamed_addr constant [25 x i8] c"SYCL/device requirements\00"
// CHECK-DAG: @SYCL_PropSetName.4 = internal unnamed_addr constant [22 x i8] c"SYCL/kernel param opt\00"
// CHECK-DAG: @__sycl_offload_prop_sets_arr.5 = internal constant [3 x %_pi_device_binary_property_set_struct] [%_pi_device_binary_property_set_struct { ptr @SYCL_PropSetName, ptr @__sycl_offload_prop_sets_arr, ptr getelementptr ([1 x %_pi_device_binary_property_struct], ptr @__sycl_offload_prop_sets_arr, i64 0, i64 1) }, %_pi_device_binary_property_set_struct { ptr @SYCL_PropSetName.3, ptr @__sycl_offload_prop_sets_arr.2, ptr getelementptr ([1 x %_pi_device_binary_property_struct], ptr @__sycl_offload_prop_sets_arr.2, i64 0, i64 1) }, %_pi_device_binary_property_set_struct { ptr @SYCL_PropSetName.4, ptr null, ptr null }]
// CHECK-DAG: @.sycl_offloading.0.data = internal unnamed_addr constant [912 x i8]
// CHECK-DAG: @.sycl_offloading.0.data = internal unnamed_addr constant [952 x i8]
// CHECK-DAG: @__sycl_offload_entry_name = internal unnamed_addr constant [25 x i8] c"_ZTSZ4mainE11fake_kernel\00"
// CHECK-DAG: @__sycl_offload_entries_arr = internal constant [1 x %struct.__tgt_offload_entry] [%struct.__tgt_offload_entry { i64 0, i16 1, i16 4, i32 0, ptr null, ptr @__sycl_offload_entry_name, i64 0, i64 0, ptr null }]
// CHECK-DAG: @.sycl_offloading.0.info = internal local_unnamed_addr constant [2 x i64] [i64 ptrtoint (ptr @.sycl_offloading.0.data to i64), i64 912], section ".tgtimg", align 16
// CHECK-DAG: @.sycl_offloading.0.info = internal local_unnamed_addr constant [2 x i64] [i64 ptrtoint (ptr @.sycl_offloading.0.data to i64), i64 952], section ".tgtimg", align 16
// CHECK-DAG: @llvm.used = appending global [1 x ptr] [ptr @.sycl_offloading.0.info], section "llvm.metadata"
// CHECK-DAG: @.sycl_offloading.device_images = internal unnamed_addr constant [1 x %__sycl.tgt_device_image] [%__sycl.tgt_device_image { i16 2, i8 4, i8 0, ptr @.sycl_offloading.target.0, ptr @.sycl_offloading.opts.compile.0, ptr @.sycl_offloading.opts.link.0, ptr null, ptr null, ptr @.sycl_offloading.0.data, ptr getelementptr ([912 x i8], ptr @.sycl_offloading.0.data, i64 0, i64 912), ptr @__sycl_offload_entries_arr, ptr getelementptr ([1 x %struct.__tgt_offload_entry], ptr @__sycl_offload_entries_arr, i64 0, i64 1), ptr @__sycl_offload_prop_sets_arr.5, ptr getelementptr ([3 x %_pi_device_binary_property_set_struct], ptr @__sycl_offload_prop_sets_arr.5, i64 0, i64 3) }]
// CHECK-DAG: @.sycl_offloading.device_images = internal unnamed_addr constant [1 x %__sycl.tgt_device_image] [%__sycl.tgt_device_image { i16 2, i8 4, i8 0, ptr @.sycl_offloading.target.0, ptr @.sycl_offloading.opts.compile.0, ptr @.sycl_offloading.opts.link.0, ptr null, ptr null, ptr @.sycl_offloading.0.data, ptr getelementptr ([952 x i8], ptr @.sycl_offloading.0.data, i64 0, i64 952), ptr @__sycl_offload_entries_arr, ptr getelementptr ([1 x %struct.__tgt_offload_entry], ptr @__sycl_offload_entries_arr, i64 0, i64 1), ptr @__sycl_offload_prop_sets_arr.5, ptr getelementptr ([3 x %_pi_device_binary_property_set_struct], ptr @__sycl_offload_prop_sets_arr.5, i64 0, i64 3) }]
// CHECK-DAG: @.sycl_offloading.descriptor = internal constant %__sycl.tgt_bin_desc { i16 1, i16 1, ptr @.sycl_offloading.device_images, ptr null, ptr null }
// CHECK-DAG: @llvm.global_ctors = {{.*}} { i32 1, ptr @sycl.descriptor_reg, ptr null }]
// CHECK-DAG: @llvm.global_dtors = {{.*}} { i32 1, ptr @sycl.descriptor_unreg, ptr null }]
Expand Down
Loading