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

Ensure elision of require_one_based_indexing with high-dim array views #53091

Merged
merged 18 commits into from
Jan 30, 2024

Conversation

pablosanjose
Copy link
Contributor

Closes #49332

Here I try to make the life of the compiler a bit easier by reorganising the dispatch structure of has_offset_axes (#45260) into two layers. Avoiding the self-reference of the has_offset_axes(As...) method seems to allow the elision of the call in my machine, which fixes #49332 by fully eliminating require_one_based_indexing even for the problematic case of views of 4+ dimensional arrays.

@pablosanjose
Copy link
Contributor Author

This fixed one case but broke others. Tweaks are needed still

@pablosanjose pablosanjose marked this pull request as draft January 28, 2024 21:10
@pablosanjose pablosanjose marked this pull request as ready for review January 29, 2024 09:52
@pablosanjose
Copy link
Contributor Author

The first fix was not the correct one. With the last commit we simplify the lispy recursion of has_offset_axes(As...) without using _any_tuple. This assumes that has_offset_axes(A) is always a Bool, and never Missing. If this assumption holds, the new and old code should be equivalent. Otherwise, if we hit a has_offset_axes(A)::Missing both master and PR would throw a TypeError: non-boolean (Missing) used in boolean context, but at different points of the call chain (require_one_based_indexing vs has_offset_axes).

CC: @N5N3

@N5N3 N5N3 added the domain:arrays [a, r, r, a, y, s] label Jan 29, 2024
@N5N3
Copy link
Member

N5N3 commented Jan 29, 2024

LGTM, I guess the doc string could be updated? (| -> ||)

@pablosanjose
Copy link
Contributor Author

I guess the doc string could be updated? (| -> ||)

Done, thanks!

@pablosanjose pablosanjose added the backport 1.10 Change should be backported to the 1.10 release label Jan 29, 2024
base/abstractarray.jl Outdated Show resolved Hide resolved
Co-authored-by: Denis Barucic <barucic.d@gmail.com>
@N5N3 N5N3 merged commit 9edf1dd into JuliaLang:master Jan 30, 2024
4 of 7 checks passed
KristofferC pushed a commit that referenced this pull request Feb 6, 2024
…ews (#53091)

Closes #49332

---------

Co-authored-by: Denis Barucic <barucic.d@gmail.com>
(cherry picked from commit 9edf1dd)
@KristofferC KristofferC mentioned this pull request Feb 6, 2024
9 tasks
@KristofferC KristofferC removed the backport 1.10 Change should be backported to the 1.10 release label Feb 6, 2024
KristofferC added a commit that referenced this pull request Feb 6, 2024
A few stragglers.

Backported PRs:
- [x] #53091 <!-- Ensure elision of `require_one_based_indexing` with
high-dim array views -->
- [x] #53117 <!-- Try to fix incorrect documentation of `nthreads` -->
- [x] #52855 <!-- Fix variable name in scaling an `AbstractTriangular`
with zero alpha -->
- [x] #52952 <!-- [REPLCompletions] enable completions for `using
Module.Inner|` -->
- [x] #53101 <!-- Inplace transpose for unit Triangular may skip
diagonal -->

Need manual backport:
- [ ] #52505 <!-- fix alignment of emit_unbox_store copy -->

Non-merged PRs with backport label:
- [ ] #53125 <!-- coverage: count coverage where explicitly requested by
inference only -->
- [ ] #52694 <!-- Reinstate similar for AbstractQ for backward
compatibility -->
- [ ] #51479 <!-- prevent code loading from lookin in the versioned
environment when building Julia -->
Drvi pushed a commit to RelationalAI/julia that referenced this pull request Jun 7, 2024
…ews (JuliaLang#53091)

Closes JuliaLang#49332

---------

Co-authored-by: Denis Barucic <barucic.d@gmail.com>
(cherry picked from commit 9edf1dd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:arrays [a, r, r, a, y, s]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression: mul! of views allocates in v1.9+ for arrays of 4 or higher dimension, not in v1.8.5
4 participants