Skip to content

[SYCL] Add support for NVPTX device printf #4293

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

Jump to bottom
Merged
merged 3 commits into from
Aug 17, 2021
Merged

[SYCL] Add support for NVPTX device printf #4293

merged 3 commits into from
Aug 17, 2021

Conversation

jchlanda
Copy link
Contributor

@jchlanda jchlanda commented Aug 9, 2021 •
edited
Loading

Use ::printf when not compiling for __SPIR__, this allows the use of EmitNVPTXDevicePrintfCallExpr which packs the var args and dispatches to CUDA's vprintf.

Fixes #1154

@jchlanda jchlanda requested a review from sergey-semenov August 9, 2021 10:07
bader
bader reviewed Aug 9, 2021
View reviewed changes
@jchlanda
Copy link
Contributor Author

jchlanda commented Aug 9, 2021

Looks like the build might be broken, there are multiple failures to do with hipMemsetD[8|16|32]Async that were not introduced in this PR.

@jchlanda jchlanda force-pushed the ptx_printf branch 3 times, most recently from fe58b68 to e7b4173 Compare August 9, 2021 12:53
@jchlanda jchlanda requested a review from bader August 9, 2021 12:54
bader
bader reviewed Aug 9, 2021
View reviewed changes
Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

Looks much simpler. Just one more suggestion.
Could you update PR title, please?

@jchlanda jchlanda changed the title [SYCL] Use __builtin_printf for sycl devices [SYCL] Add support for NVPTX device printf Aug 9, 2021
bader
bader previously approved these changes Aug 9, 2021
View reviewed changes
Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

Great. Thank you!

@bader bader added the cuda CUDA back-end label Aug 9, 2021
@jchlanda
Copy link
Contributor Author

Is the Precommit failure something that I would need to look into?

elizabethandrews
elizabethandrews reviewed Aug 11, 2021
View reviewed changes
Copy link
Contributor

@elizabethandrews elizabethandrews left a comment

Choose a reason for hiding this comment

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

Can we add a FE test to verify change?

@bader
Copy link
Contributor

bader commented Aug 11, 2021

Is the Precommit failure something that I would need to look into?

I don't think so. Precommit doesn't run any tests on CUDA back-end and the failure in this particular is seems to be known flaky issue - it's the same as the second issue in this comment #4281 (comment).

@jchlanda
[SYCL] Add support for NVPTX device printf
d4dffe4 
Use `::printf` when not compiling for `__SPIR__`, this allows the use of
`EmitNVPTXDevicePrintfCallExpr` which packs the var args and dispatches
to CUDA's `vprintf`.

Fixes intel#1154
@jchlanda
Copy link
Contributor Author

Can we add a FE test to verify change?

Added in f7bbcc6

bader
bader previously approved these changes Aug 13, 2021
View reviewed changes
@bader
Copy link
Contributor

bader commented Aug 16, 2021

@sergey-semenov, @intel/llvm-reviewers-runtime, ping.

sergey-semenov
sergey-semenov previously approved these changes Aug 16, 2021
View reviewed changes
@AerialMantis
Copy link
Contributor

@premanandrao your suggestion makes sense, unfortunately @jchlanda is on leave for a few weeks, would you be okay with this change being made in a follow up pull request?

bader
bader reviewed Aug 17, 2021
View reviewed changes
@bader bader dismissed stale reviews from sergey-semenov, elizabethandrews, and themself via c8b0ea2 August 17, 2021 10:08
@bader
Copy link
Contributor

bader commented Aug 17, 2021

@premanandrao your suggestion makes sense, unfortunately @jchlanda is on leave for a few weeks, would you be okay with this change being made in a follow up pull request?

I've applied the suggestion. I think we can merge it, if tests pass.

@AerialMantis
Copy link
Contributor

AerialMantis commented Aug 17, 2021 •
edited by bader
Loading

I've applied the suggestion. I think we can merge it, if tests pass.

That works too, thanks @bader

@bader bader merged commit 4af2eb5 into intel:sycl Aug 17, 2021
@masterleinad masterleinad mentioned this pull request Aug 17, 2021
Merged
AlexeySachkov
AlexeySachkov reviewed Nov 11, 2021
View reviewed changes
clang/lib/Sema/SemaSYCL.cpp
Comment on lines +423 to +424
if (FD->getBuiltinID() == Builtin::BIprintf)
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Tagging @AaronBallman, @elizabethandrews and @premanandrao for awareness: this if should probably be extended to check if we are compiling for CUDA or not.

The thing is that printf built-in as-is won't be properly converted into SPIR-V and will cause JIT compilation errors on OpenCL backends.

Copy link
Contributor

Choose a reason for hiding this comment

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

Changing it to check for CUDA makes sense to me, good catch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexeySachkov AlexeySachkov AlexeySachkov left review comments

@bader bader bader approved these changes

@romanovvlad romanovvlad romanovvlad approved these changes

@sergey-semenov sergey-semenov sergey-semenov left review comments

@elizabethandrews elizabethandrews elizabethandrews left review comments

@AaronBallman AaronBallman Awaiting requested review from AaronBallman

@premanandrao premanandrao Awaiting requested review from premanandrao

Assignees
No one assigned
Labels
cuda CUDA back-end
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[CUDA] printfs results in ptxas error: Call has wrong number of parameters
9 participants
@jchlanda @bader @AerialMantis @AaronBallman @AlexeySachkov @romanovvlad @sergey-semenov @premanandrao @elizabethandrews