Skip to content
This repository was archived by the owner on Mar 28, 2023. It is now read-only.

[SYCL] Enable level-zero-link-flags and fix kernel_bundle_api #806

Merged
merged 11 commits into from
Feb 28, 2022

Conversation

HabKaffee
Copy link

This patch enabling KernelAndProgram/level-zero-link-flags.cpp due to intel/llvm#5476

@HabKaffee HabKaffee requested a review from a team as a code owner February 7, 2022 08:14
@HabKaffee
Copy link
Author

/verify with intel/llvm#5476

@HabKaffee HabKaffee marked this pull request as draft February 7, 2022 08:31
@HabKaffee
Copy link
Author

/verify intel/llvm#5476

@HabKaffee
Copy link
Author

/verify with intel/llvm#5476

@HabKaffee HabKaffee changed the title [SYCL] Enable level-zero-link-flags.cpp [SYCL] Enable level-zero-link-flags and fix kernel_bundle_api Feb 7, 2022
@HabKaffee
Copy link
Author

/verify with intel/llvm#5476

@alexbatashev alexbatashev requested a review from s-kanaev February 7, 2022 10:18
// Use of per-kernel device code split and linking the bundle with all images
// involved leads to multiple definition of AssertHappened structure due each
// device image is statically linked against fallback libdevice.
// RUN: %clangxx -DSYCL_DISABLE_FALLBACK_ASSERT=1 -fsycl -fsycl-device-code-split=per_kernel -fsycl-targets=%sycl_triple %s -o %t.out
// RUN: %clangxx -fsycl -fsycl-device-code-split=per_kernel -fsycl-targets=%sycl_triple %s -o %t.out
Copy link

Choose a reason for hiding this comment

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

I'm curious, how is this related to the bug that was fixed in intel/llvm#5476?

Copy link
Author

Choose a reason for hiding this comment

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

This test started failing after I started check intel/llvm#5476 . Moreover, there was a comment line //Disable fallback assert here until online-support is fixed. I can guess, that it is about fixed bug.

Copy link
Author

Choose a reason for hiding this comment

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

I resolved the problem. Basic/kernel_bundle/kernel_bundle_api was affected indirectly. Before intel/llvm#5476 it received a nullptr as a link option, but now here is passed an empty string.

@HabKaffee
Copy link
Author

/verify with intel/llvm#5476

@HabKaffee
Copy link
Author

/verify with intel/llvm#5476

@HabKaffee HabKaffee marked this pull request as ready for review February 8, 2022 11:06
@@ -1,4 +1,3 @@
// Disable fallback assert here until online-support is fixed.
Copy link

Choose a reason for hiding this comment

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

It might be worth asking @s-kanaev if this comment should be removed. It looks to me like the test still has fallback assert disabled, so maybe the comment should remain until it is enabled.

Copy link
Author

Choose a reason for hiding this comment

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

I'll ask about your concern.

Choose a reason for hiding this comment

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

Fallback assert is now disabled by default. See intel/llvm#4694.
Hence, this comment might be removed along with -DSYCL_DISABLE_FALLBACK_ASSERT=1 in build line.
It'll be responsibility of the one who enables the fallback assert to verify it won't impact other tests.

P.S.: Sorry for late response.

@HabKaffee
Copy link
Author

/verify with intel/llvm#5476

@HabKaffee
Copy link
Author

/verify with intel/llvm#5476

1 similar comment
@HabKaffee
Copy link
Author

/verify with intel/llvm#5476

@HabKaffee
Copy link
Author

intel/llvm#5476

1 similar comment
@HabKaffee
Copy link
Author

intel/llvm#5476

@HabKaffee
Copy link
Author

/verify with intel/llvm#5476

@HabKaffee
Copy link
Author

/verify with intel/llvm#5476

@HabKaffee
Copy link
Author

@s-kanaev @alexbatashev Could you, please, take a look on this PR?

Copy link

@s-kanaev s-kanaev 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 seems legit.

@HabKaffee
Copy link
Author

/verify with intel/llvm#5476

@vladimirlaz vladimirlaz merged commit 6f29028 into intel:intel Feb 28, 2022
myler pushed a commit to myler/llvm-test-suite that referenced this pull request Apr 12, 2022
myler pushed a commit to myler/llvm-test-suite that referenced this pull request Jun 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants