-
Notifications
You must be signed in to change notification settings - Fork 803
[NewOffloadDriver] Add AOT device image selection test for new offloading model #20976
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.
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
[NewOffloadDriver] Add AOT device image selection test for new offloading model #20976
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intended that we will run this test in full on Windows and only compile the tests but not run them on Linux?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this test is currently only being run on Gen 12 Linux in CI, where it's failing for the new offloading model (CI result for new offloading model can be found at https://github.com/intel/llvm/actions/runs/19845783730/job/56863976891?pr=20570) . Since we don't expect this test to be supported on any platform, I am thinking maybe changing it to // UNSUPPORTED: * would likely be a better approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if its UNSUPPORTED: * is it better to just delete the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure about this approach because in our previous "new offloading model meeting and sync ..." meeting, I think @YuriPlyakhin mentioned that we should mark this test as unsupported and add an explanation. This is because the feature being tested (AOT compilation for GPU without specifying the device name) is supported in the old offloading model but not in the new offloading model (that was a mistake in the old offloading model and should not be supported). Therefore, I thought we should keep this test so that after we switch to the new offloading model, if users wonder why we no longer support this feature or this test, they can find the explanation in the test. We could also wait until @YuriPlyakhin returns later this week to see if he has additional insights.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't usually leave disabled tests for removed features, from the meeting I thought the test was still going to run in build-only or something, maybe I misunderstood
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Yury's comment below suggested that we should mark this test as UNSUPPORTED: new-offload-model, and the test will be removed after old offloading model support is removed (since this feature is supported for old offloading model but not for new offloading model, and the current default offloading model is still the old offloading model). I have created an UNSUPPORTED-TRACKER at #20988 to track this.
|
I don't think the current CI failure is caused by the change in this patch. This change only modifies test files |
YuriPlyakhin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
One nit I noticed only now: the name of the new test should be aot-multiple-devices - note the s in the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that generic SPIR-V compilation is enabled by default even when AOT GPU or CPU targets are specified.
Where are you seeing this? I tried the old offload model and it seems it only generates the explicitly specified targets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When running clang++ -fsycl -fsycl-targets=spir64_x86_64 a.o -o a.out for AOT CPU compilation, I found that clang-offload-wrapper is invoked for both -target=spir64 and -target=spir64_x86_64, even though only spir64_x86_64 is specified in fsycl-targets.
This is also what the test verifies at this line: even though spir64 is not explicitly specified in fsycl-targets, the test still see clang-offload-wrapper{{.*}} -target=spir64 to be called (verified through --check-prefix=CHECK-GENERIC).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok yeah it's because this line is using a pre-generated fat object which contains spir64, that explains it, sorry for the confusion
|
@intel/llvm-gatekeepers please consider merging |
sarnex
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks for the explanations!
In the old offloading model, we supported compiling AOT images for GPU without specifying the device name (without
-Xsycl-target-backend=spir64_gen -device pvc). In the new offloading model, this feature is no longer supported and we now enforce that device names must be specified when compiling image AOT for GPU.The following changes have been made to the SYCL E2E tests:
AOT/multiple-devices.cppis marked as UNSUPPORTEDNewOffloadDriver/aot-multiple-device.cpptest is added for the new offloading model. This test is a copy ofAOT/multiple-devices.cppwith the addition of explicit device names when compiling the AOT fat binary object.