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

Store the frontend task object in the REPL struct. #48400

Merged
merged 1 commit into from
Jan 27, 2023

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Jan 25, 2023

This makes it possible for GPU back-ends to allow so-called scalar iteration (element-wise copies of elements, often triggered by iterate in generic fallbacks, normally disallowed because it indicates that processing is happening on the CPU) in the REPL, where it is often triggered by show methods visualizing output.

If this isn't too controversial, I'd like to get this in 1.9 because it is a significant QOL improvement to GPU users.

X-ref JuliaGPU/GPUArrays.jl#446

@maleadt maleadt added the REPL Julia's REPL (Read Eval Print Loop) label Jan 25, 2023
@maleadt
Copy link
Member Author

maleadt commented Jan 26, 2023

@KristofferC and @rfourquet seem to have made the most changes to the REPL sources lately, so pinging for review.

@maleadt maleadt added the backport 1.9 Change should be backported to release-1.9 label Jan 27, 2023
@rfourquet
Copy link
Member

I must admit I don't understand the problem this would solve, so I can't really comment. This change seems fine though, although in-code comments and/or tests would be welcome such that it's not reverted accidentally in a future commit.

@maleadt
Copy link
Member Author

maleadt commented Jan 27, 2023

So GPU arrays need all their operations to be implemented using vectorized functions, which dispatch to a GPU kernel. Base however has plenty of fallback functions that perform element-wise operations (think for loops, etc). That's why we generally disallow getindex and other scalar functions, throwing an error instead.

There's one big exception to this: the display stack. We don't want to have to reimplement every show method to avoid triggering getindex, especially because performance doesn't matter there, and also because it's hard in the presence of array wrappers (view(::GPUArray) isn't a GPUArray). So ideally we want a simple way to allow scalar iteration for display code. We haven't found a way to do this, but most of the time users only display arrays from the REPL, so with this PR we can do (pseudo-code):

function getindex(::GPUArray, idx...)
    current_task() == Base.active_repl.frontend_task && return
    error("scalar getindex not allowed")
end

@vchuravy vchuravy requested a review from KristofferC January 27, 2023 13:50
@KristofferC KristofferC merged commit 87f8958 into master Jan 27, 2023
@KristofferC KristofferC deleted the tb/repl_frontend_task branch January 27, 2023 13:53
KristofferC pushed a commit that referenced this pull request Feb 1, 2023
@KristofferC KristofferC mentioned this pull request Feb 1, 2023
35 tasks
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REPL Julia's REPL (Read Eval Print Loop)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants