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.

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

[SYCLomatic][PTX] Added support for prefetch ASM migration #2579

Merged
merged 3 commits into from
Jan 7, 2025

Conversation

TejaX-Alaghari
Copy link
Contributor

Added support for prefetch PTX ASM migration

SYCL mapping is available under sycl_ext_oneapi_prefetch extension

Migrates syntax similar to here

// CHECK-NEXT: */
asm volatile ("prefetch.local.L2 [%0];" : : "l"(arr));
// CHECK: /*
// CHECK-NEXT: DPCT1053:{{.*}} Migration of device assembly code is not supported.
Copy link
Contributor

@zhimingwang36 zhimingwang36 Dec 24, 2024

Choose a reason for hiding this comment

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

is there mapping? is no, how about we just remove it with DPCT warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, these is no mapping in SYCL except for global and generic addr space prefetching.

Do you suggest to remove the unsupported combination of prefetch instructions from migrated code?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, just removing it with an DPCT warnning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this scenario, just return SYCLGenError(), no need to emit a warning msg, the current ASM migration framework will emit a warning msg for "SYCLGenError()" senario.

Proposing to retain this behavior upon suggestion from @tomflinda (ref)

Copy link
Contributor

@tomflinda tomflinda left a comment

Choose a reason for hiding this comment

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

@TejaX-Alaghari
Pls prepare the E2E test firstly, for this E2E test,
It should cover the use scenarios for the PTX instruction we can support and the verification code for this PTX instruction, and it should be run passed with CUDA version
Then migrate the E2E test to SYCL version with mappings provided from SYCL side or help function, in doing so, we can verify whether the mappings we provided are correct.

Finally, we implement the migration logic in SYCLomatic, lit test from the E2E test, also E2E test should be submitted to SYCLomatic-test repo for this PTX instruction.

@TejaX-Alaghari TejaX-Alaghari force-pushed the ptx branch 3 times, most recently from 5ff4de2 to 3ed6837 Compare December 30, 2024 01:49
@TejaX-Alaghari TejaX-Alaghari force-pushed the ptx branch 2 times, most recently from eec848c to 758c8fb Compare January 4, 2025 14:27
@TejaX-Alaghari TejaX-Alaghari force-pushed the ptx branch 2 times, most recently from 4649e9c to e742c34 Compare January 7, 2025 02:33
Copy link
Contributor

@tomflinda tomflinda left a comment

Choose a reason for hiding this comment

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

LGTM

@zhimingwang36 zhimingwang36 merged commit 09b221a into oneapi-src:SYCLomatic Jan 7, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants