Skip to content

[Driver][SYCL][NewOffload] Fix arch settings for nvptx and amd #14340

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

Merged
merged 3 commits into from
Jul 1, 2024

Conversation

mdtoguchi
Copy link
Contributor

When compiling for -fsycl-targets values of nvptx64-nvidia-cuda and amdgcn-amd-gpu, the default arch behaviors were not applied to the compilation. Updates to do the following:

  • Add default of sm_50 for nvptx64 if not provided
  • Emit diagnostic if no arch provided for amd
  • Parse -Xsycl-backend-target for offload-arch values

When compiling for -fsycl-targets values of nvptx64-nvidia-cuda and
amdgcn-amd-gpu, the default arch behaviors were not applied to the
compilation.  Updates to do the following:
 - Add default of sm_50 for nvptx64 if not provided
 - Emit diagnostic if no arch provided for amd
 - Parse -Xsycl-backend-target for offload-arch values
@mdtoguchi mdtoguchi requested a review from a team as a code owner June 28, 2024 00:55
@mdtoguchi mdtoguchi requested a review from asudarsa June 28, 2024 00:55
Copy link
Contributor

@asudarsa asudarsa left a comment

Choose a reason for hiding this comment

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

I tested the changes for the new offloading model on several failing SYCL E2E tests on an AMD machine and the fix resolves all of those failures. Test added looks fine.

Thanks much for the fix.

@mdtoguchi mdtoguchi requested a review from hchilama June 28, 2024 15:42
@mdtoguchi
Copy link
Contributor Author

@intel/llvm-gatekeepers, This should be ready for merge - please take a look.

@asudarsa
Copy link
Contributor

We should really be not running CI testing for comment updates...:-(

@martygrant
Copy link
Contributor

@mdtoguchi this is awaiting approval from @srividya-sundaram

@asudarsa
Copy link
Contributor

asudarsa commented Jul 1, 2024

We have approval from a driver team reviewer, This change unblocks development by fixing a large number of test failures. So, I ill merge it now. @srividya-sundaram, please feel free to reach out to me or @mdtoguchi for any post-commit changes if required.

Thanks
Sincerely

@asudarsa asudarsa merged commit c99522b into intel:sycl Jul 1, 2024
14 checks passed
@srividya-sundaram
Copy link
Contributor

Sorry, I missed the notification for approval request.

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.

5 participants