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

Allow sorting of tuples of numbers #1196

Merged
merged 3 commits into from
Jan 3, 2022
Merged

Conversation

mcabbott
Copy link
Contributor

@mcabbott mcabbott commented Oct 10, 2021

This allows sort to work on arrays of tuples, motivated by partialsortperm(x; dims)-like things in FluxML/NNlib.jl#353

It also adds the simplest implementation of sortperm using that. Not sure if this is too crude for the package to want to keep? Needs tests, if it is wanted.

src/sorting.jl Outdated Show resolved Hide resolved
src/sorting.jl Outdated Show resolved Hide resolved
@maleadt
Copy link
Member

maleadt commented Oct 12, 2021

Using zero for an unused value seems wrong, as noticed here it doesn't work for all types, and it's also unnecessary since it just represents an unused value (but we need to do this in order to avoid deadlocks).

What about:

julia> f(i, a) = i ? @inbounds(a[]) : Ref{Int}()[]
f (generic function with 2 methods)

julia> f(false, [1])
140526431453856

julia> f(true, [1])
1

julia> @code_llvm f(true, [1])
;  @ REPL[11]:1 within `f`
define i64 @julia_f_235(i8 zeroext %0, {}* nonnull align 16 dereferenceable(40) %1) #0 {
top:
  %2 = and i8 %0, 1
  %.not = icmp eq i8 %2, 0
  br i1 %.not, label %L28, label %L23

L23:                                              ; preds = %top
; ┌ @ abstractarray.jl:1218 within `getindex`
; │┌ @ abstractarray.jl:1245 within `_getindex`
; ││┌ @ array.jl:839 within `getindex`
     %3 = bitcast {}* %1 to i64**
     %4 = load i64*, i64** %3, align 8
     %5 = load i64, i64* %4, align 8
; └└└
  ret i64 %5

L28:                                              ; preds = %top
  ret i64 undef
}

Bit ugly, but does result in the expected undef.

@maleadt maleadt added cuda array Stuff about CuArray. enhancement New feature or request needs changes Changes are needed. needs tests Tests are requested. labels Oct 12, 2021
@mcabbott
Copy link
Contributor Author

Ok, I did not look closely enough to figure out what the zero was or wasn't being used for. If unused, then a generic undef does sound better.

@maleadt maleadt changed the title Allow sorting of tuples, and perhaps add sortperm Allow sorting of tuples of numbers Dec 9, 2021
@maleadt
Copy link
Member

maleadt commented Dec 9, 2021

Rebased. Removed the sortperm since that has been introduced by another PR. Also replaced the one/zero calls to the above Ref trick since those values are unused anyway.

src/sorting.jl Outdated Show resolved Hide resolved
@maleadt maleadt removed needs tests Tests are requested. needs changes Changes are needed. labels Dec 10, 2021
@maleadt
Copy link
Member

maleadt commented Dec 10, 2021

Turns out we can just do the naive thing (on 1.7, at least): define a variable conditionally and use it under the same condition so that LLVM can optimize the throw(UndefVarError) away. Let's see how CI things about this.

@mcabbott
Copy link
Contributor Author

Thanks for picking this up!

@maleadt maleadt force-pushed the sortperm branch 4 times, most recently from 520ca81 to a264126 Compare December 21, 2021 15:24
@codecov
Copy link

codecov bot commented Dec 21, 2021

Codecov Report

Merging #1196 (5a9611f) into master (bf26270) will increase coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1196   +/-   ##
=======================================
  Coverage   78.94%   78.95%           
=======================================
  Files         119      119           
  Lines        8650     8649    -1     
=======================================
  Hits         6829     6829           
+ Misses       1821     1820    -1     
Impacted Files Coverage Δ
src/sorting.jl 24.14% <0.00%> (+0.05%) ⬆️

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 94ba745...5a9611f. Read the comment docs.

@maleadt maleadt force-pushed the sortperm branch 6 times, most recently from 7395aba to 6504ad4 Compare December 21, 2021 16:24
@maleadt maleadt force-pushed the sortperm branch 9 times, most recently from 12fb49e to 431d318 Compare December 21, 2021 18:29
@maleadt maleadt merged commit 363f92d into JuliaGPU:master Jan 3, 2022
@mcabbott mcabbott deleted the sortperm branch January 3, 2022 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cuda array Stuff about CuArray. enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants