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

Make ndims and parent inferrable #33

Merged
merged 1 commit into from
Dec 8, 2020

Conversation

martenlienen
Copy link
Contributor

Previously, the @isdefined conditionals prevented julia from inferring the type of these
methods at compile time. With this change broadcast operations in CUDA.jl like C .- a'
can now be fully inferred which was previously impossible because the
BroadcastStyle (which uses ndims internally) could not be decided at compile time.

@maleadt
Copy link
Member

maleadt commented Dec 8, 2020

Thanks. I guess this doesn't hurt, but it reminds me that we really shouldn't be using these methods here: #30

@maleadt
Copy link
Member

maleadt commented Dec 8, 2020

Maybe now's a good time as ever to bite that bullet; with the 1.6 upgrade requiring breaking upgrades to most downstream packages anyway.

@martenlienen
Copy link
Contributor Author

I think I don't understand the whole context. Does that mean that this PR is obsolete? Or are you proposing a different change? Anyway it is nice that Julia can now type infer broadcasted CUDA expressions, at least in my local dev version.

@maleadt
Copy link
Member

maleadt commented Dec 8, 2020

No, this is still useful. Let me first fix CI though.

Previously, the `@isdefined` conditionals prevented julia from inferring the type of these
methods at compile time. With this change broadcast operations in CUDA.jl like `C .- a'`
can now be fully inferred which was previously impossible because the
BroadcastStyle (which uses ndims internally) could not be decided at compile time.
@codecov
Copy link

codecov bot commented Dec 8, 2020

Codecov Report

Merging #33 (1b99b81) into master (1edd29c) will increase coverage by 5.71%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #33      +/-   ##
==========================================
+ Coverage   62.85%   68.57%   +5.71%     
==========================================
  Files           3        3              
  Lines          35       35              
==========================================
+ Hits           22       24       +2     
+ Misses         13       11       -2     
Impacted Files Coverage Δ
src/wrappers.jl 70.37% <33.33%> (+7.40%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1edd29c...1b99b81. Read the comment docs.

@maleadt maleadt merged commit 0594757 into JuliaGPU:master Dec 8, 2020
@marius311
Copy link
Contributor

Just went down a rabbit hole of why my view(::CuArray) broadcasts weren't inferring, which ultimately led me to this PR, which fixes it. Much appreciated if this could be tagged in the near future so I can add it as a compat bound.

@maleadt
Copy link
Member

maleadt commented Jan 4, 2021

JuliaRegistries/General#27297

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