Skip to content

Conversation

@brenhinkeller
Copy link
Contributor

@brenhinkeller brenhinkeller commented Feb 14, 2022

This would help enable LoopVectorization of custom array types that subtype <: DenseArray <: AbstractArray. Since DenseArray is defined to be column-major

DenseArray is an abstract subtype of AbstractArray intended to include all arrays where elements are stored contiguously in column-major order

this should be kosher AFAIU.

@ChrisRackauckas
Copy link
Member

https://github.com/JuliaArrays/ArrayInterface.jl/runs/5182799084?check_suite_focus=true#step:6:213
https://github.com/JuliaArrays/ArrayInterface.jl/runs/5182799146?check_suite_focus=true#step:6:172

@Tokazama We should revert on Friday if this all doesn't get fixed. Downstream shouldn't be failing for more than a week or two without a downstream PR.

@chriselrod
Copy link
Collaborator

chriselrod commented Feb 14, 2022

I think this makes sense, but as this is expressly for LoopVectorization support, I'd like to see
JuliaSIMD/LoopVectorization.jl#383
merged first so we can run downstream tests on it.

But LV could also depend on this and explicitly test and document a simple API for adding LV support to custom array types.

@codecov
Copy link

codecov bot commented Feb 14, 2022

Codecov Report

Merging #243 (e1cf4dc) into master (b2304bd) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #243   +/-   ##
=======================================
  Coverage   89.14%   89.14%           
=======================================
  Files          11       11           
  Lines        1751     1751           
=======================================
  Hits         1561     1561           
  Misses        190      190           
Impacted Files Coverage Δ
src/stridelayout.jl 87.18% <100.00%> (ø)

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 b2304bd...e1cf4dc. Read the comment docs.

@brenhinkeller
Copy link
Contributor Author

No rush on my end! This'll ultimately hopefully support LV of MallocArrays from StaticTools.jl but I've got lots of other things to worry about in the meanwhile, so can wait and see how JuliaSIMD/LoopVectorization.jl#383 / the downstream fixes play out first

@brenhinkeller
Copy link
Contributor Author

Any progress on the downstream stuff lately @Tokazama? I'd try to help, but a lot of the LV internals are still beyond me

@Tokazama
Copy link
Member

Sorry I've been recovering from COVID the last week and haven't been very active on GitHub. LoopVectorization has some odd failures that I don't understand why they occur with the change in question.

@brenhinkeller
Copy link
Contributor Author

Is this potentially back on the table now? Looks like downstream tests are passing

@brenhinkeller
Copy link
Contributor Author

Oh hmm, are these expected? It was all green before the rerun.. Seems like something is broken on Nightly but that might be unrelated? I can't see the logs though due to some permissions setting.

@Tokazama
Copy link
Member

Tokazama commented Mar 11, 2022

This is an inference issue we've ran into on nightly that I'm pretty sure is unrelated

@Tokazama
Copy link
Member

@brenhinkeller, if you think this should be released in a new update then there needs to be an update to Project.toml too.

@brenhinkeller
Copy link
Contributor Author

Version updated!

@Tokazama Tokazama merged commit 0d039cd into JuliaArrays:master Mar 13, 2022
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.

4 participants