Skip to content

Conversation

pablosanjose
Copy link
Contributor

Closes JuliaLang/LinearAlgebra.jl#998

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 JuliaLang/LinearAlgebra.jl#998 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 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
Co-authored-by: Denis Barucic <[email protected]>
@N5N3 N5N3 merged commit 9edf1dd into JuliaLang:master Jan 30, 2024
KristofferC pushed a commit that referenced this pull request Feb 6, 2024
…ews (#53091)

Closes #49332

---------

Co-authored-by: Denis Barucic <[email protected]>
(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 #49332

---------

Co-authored-by: Denis Barucic <[email protected]>
(cherry picked from commit 9edf1dd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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