Fix `sum()` and `prod()` for tuples #41510

petvana · 2021-07-08T12:50:22Z

This PR aims to fix #39182 and #39183 by using the universal implementation of prod and sum from

Line 588 in 97f817a

prod(a; kw...) = mapreduce(identity, mul_prod, a; kw...)

However, the file abstractarray.jl is included way sooner, and it is crucial to have already a simplified version of prod function. ~~To distinguish the problematic locations (and not redefine the functions), I renamed these occurrences of the function to _prod_simple (not exported from Base).~~

The TODO was left in #22825 (@TotalVerb).

EDIT: The separate function _prod_simple is not necessary because we can specify a simplified version or prod only for a system-wide Int type that is sufficient to compile Base.

prod(x::Tuple{}) = 1
# This is consistent with the regular prod because there is no need for size promotion
# if all elements in the tuple are of system size.
prod(x::Tuple{Int, Vararg{Int}}) = *(x...)

Although the implementations are different, they lead to the same binary code for tuples containing ~~UInt and~~ Int.

julia> a = (1,2,3)
(1, 2, 3)

# Simplified version for tuples containing Int only
julia> prod_simplified(x::Tuple{Int, Vararg{Int}}) = *(x...)

julia> @code_native prod_simplified(a)
	.text
; ┌ @ REPL[1]:1 within `prod_simplified'
; │┌ @ operators.jl:560 within `*' @ int.jl:88
	movq	8(%rdi), %rax
	imulq	(%rdi), %rax
	imulq	16(%rdi), %rax
; │└
	retq
	nop
; └

# Regular prod without the simplification
julia> @code_native prod(a)
        .text
; ┌ @ reduce.jl:588 within `prod`
; │┌ @ reduce.jl:588 within `#prod#247`
; ││┌ @ reduce.jl:289 within `mapreduce`
; │││┌ @ reduce.jl:289 within `#mapreduce#240`
; ││││┌ @ reduce.jl:162 within `mapfoldl`
; │││││┌ @ reduce.jl:162 within `#mapfoldl#236`
; ││││││┌ @ reduce.jl:44 within `mapfoldl_impl`
; │││││││┌ @ reduce.jl:48 within `foldl_impl`
; ││││││││┌ @ tuple.jl:276 within `_foldl_impl`
; │││││││││┌ @ operators.jl:613 within `afoldl`
; ││││││││││┌ @ reduce.jl:81 within `BottomRF`
; │││││││││││┌ @ reduce.jl:38 within `mul_prod`
; ││││││││││││┌ @ int.jl:88 within `*`
        movq    8(%rdi), %rax
        imulq   (%rdi), %rax
; │││││││││└└└└
; │││││││││┌ @ operators.jl:614 within `afoldl`
; ││││││││││┌ @ reduce.jl:81 within `BottomRF`
; │││││││││││┌ @ reduce.jl:38 within `mul_prod`
; ││││││││││││┌ @ int.jl:88 within `*`
        imulq   16(%rdi), %rax
; │└└└└└└└└└└└└
        retq
        nop
; └

JeffBezanson · 2021-07-08T16:28:33Z

base/tuple.jl


 # NOTE: should remove, but often used on array sizes
 # TODO: this is inconsistent with the regular prod in cases where the arguments
 # require size promotion to system size.
-prod(x::Tuple{}) = 1
-prod(x::Tuple{Any, Vararg{Any}}) = *(x...)
+_prod_simple(x) = prod(x)


Is this method really needed? It seems to me in any case this is used, the caller could just call prod instead?

These two functions are not equal, and the idea was to remove incorrect code from prod and clearly state that _prod_simple is not correct at edge cases for Tuple.

First, I have tried it without this alias, but _prod_simple is sometimes called for an array as an argument in abstractarray.jl. Therefore _prod_simple just calls prod for any type except for Tuple. The new function seems necessary because prod is not yet included from reduce.jl, and bootstrap fails if removed.

@JeffBezanson Thanks for the comment. I've found a way not to define a separate method and updated the PR accordingly. Now, it seems much cleaner, and both TODOs are fully resolved.

JeffBezanson · 2021-07-08T16:28:53Z

Welcome, and thanks! Very nice first PR!

petvana · 2021-08-25T13:45:51Z

@NHDaly Would you mind checking this bugfix since you've already commented on both issues?

NHDaly

Yeah, LGTM! Thanks for the ping. 👍

base/tuple.jl

test/tuple.jl

NHDaly · 2021-08-26T20:56:22Z

I've left a couple more comments, and then it looks ready to merge. Can you address them and then rebase against master one more time? thanks!

Co-authored-by: Nathan Daly <[email protected]>

petvana · 2021-08-26T23:57:45Z

@NHDaly Thanks for rapid response. I've committed all proposed changes, rebased, and CI seems happy (at least so far). Should we also consider backporting to v1.7 (not v1.6) as bugfix? Besides, the PR should close both issues, but i cannot link the second one right now.

petvana · 2021-09-02T09:00:03Z

@NHDaly Should I ping someone to consider merging?

NHDaly

Thanks @petvana LGTM, sorry for the delay. Agreed that CI looks good. :)

I'll go ahead and merge this now!

NHDaly · 2021-09-02T19:48:54Z

backporting to v1.7 (not v1.6) as bugfix

Is there any particular reason why not to backport to 1.6? Since this is a bugfix, and 1.6 is going to be the next LTS, i believe we are meant to backport all bugfixes to 1.6 as well.

I'll mark it backport-1.6 for now until you comment! 😊 Thanks :)

petvana · 2021-09-02T20:04:48Z

First, usage of prod was changed in #41201, and thus I'm not sure if it is 1.6 compatible fix (can be checked by CI easily). Secondly, I don't like the idea of breaking anyone's code because it changes return types significantly, and tuple is a fundamental data type. On the other hand, I guess you run pkgeval on every release. So, I'm ok with backporting to v1.6.

This PR aims to fix #39182 and #39183 by using the universal implementation of `prod` and `sum` from https://github.com/JuliaLang/julia/blob/97f817a379b0c3c5f9bb803427fe88a018ebfe18/base/reduce.jl#L588 However, the file `abstractarray.jl` is included way sooner, and it is crucial to have already a simplified version of `prod` function. We can specify a simplified version or `prod` only for a system-wide `Int` type that is sufficient to compile `Base`. ``` julia prod(x::Tuple{}) = 1 # This is consistent with the regular prod because there is no need for size promotion # if all elements in the tuple are of system size. prod(x::Tuple{Int, Vararg{Int}}) = *(x...) ``` Although the implementations are different, they lead to the same binary code for tuples containing ~~`UInt` and~~ `Int`. ``` julia julia> a = (1,2,3) (1, 2, 3) # Simplified version for tuples containing Int only julia> prod_simplified(x::Tuple{Int, Vararg{Int}}) = *(x...) julia> @code_native prod_simplified(a) .text ; ┌ @ REPL[1]:1 within `prod_simplified' ; │┌ @ operators.jl:560 within `*' @ int.jl:88 movq 8(%rdi), %rax imulq (%rdi), %rax imulq 16(%rdi), %rax ; │└ retq nop ; └ ``` ``` julia # Regular prod without the simplification julia> @code_native prod(a) .text ; ┌ @ reduce.jl:588 within `prod` ; │┌ @ reduce.jl:588 within `#prod#247` ; ││┌ @ reduce.jl:289 within `mapreduce` ; │││┌ @ reduce.jl:289 within `#mapreduce#240` ; ││││┌ @ reduce.jl:162 within `mapfoldl` ; │││││┌ @ reduce.jl:162 within `#mapfoldl#236` ; ││││││┌ @ reduce.jl:44 within `mapfoldl_impl` ; │││││││┌ @ reduce.jl:48 within `foldl_impl` ; ││││││││┌ @ tuple.jl:276 within `_foldl_impl` ; │││││││││┌ @ operators.jl:613 within `afoldl` ; ││││││││││┌ @ reduce.jl:81 within `BottomRF` ; │││││││││││┌ @ reduce.jl:38 within `mul_prod` ; ││││││││││││┌ @ int.jl:88 within `*` movq 8(%rdi), %rax imulq (%rdi), %rax ; │││││││││└└└└ ; │││││││││┌ @ operators.jl:614 within `afoldl` ; ││││││││││┌ @ reduce.jl:81 within `BottomRF` ; │││││││││││┌ @ reduce.jl:38 within `mul_prod` ; ││││││││││││┌ @ int.jl:88 within `*` imulq 16(%rdi), %rax ; │└└└└└└└└└└└└ retq nop ; └ ``` (cherry picked from commit bada80c)

KristofferC · 2021-09-06T12:46:36Z

Looking at PkgEval, I don't think this should go into 1.6. It changes things a bit too much and causes some test errors. Also, https://s3.amazonaws.com/julialang-reports/nanosoldier/pkgeval/by_hash/4651333_vs_7c45ff0/ApproxFunBase.1.6.3-pre-7c1cb9f5343.log seems to have been caused by this (inference regression).

petvana · 2021-09-06T13:56:29Z

Looking at PkgEval, I don't think this should go into 1.6. It changes things a bit too much and causes some test errors.

That is exactly what my concerns were about. Also, the bugfix is not as critical.

KristofferC · 2021-09-08T10:14:41Z

While this is a bugfix, the effect it had on some packages also makes me a bit hesitant to backport this to 1.7 which is so late in the release cycle. I think it is better if this gets to live on master for a while before we commit it to a release.

NHDaly · 2021-09-26T19:37:13Z

Thanks both, makes sense to me 👍

This PR aims to fix JuliaLang#39182 and JuliaLang#39183 by using the universal implementation of `prod` and `sum` from https://github.com/JuliaLang/julia/blob/97f817a379b0c3c5f9bb803427fe88a018ebfe18/base/reduce.jl#L588 However, the file `abstractarray.jl` is included way sooner, and it is crucial to have already a simplified version of `prod` function. We can specify a simplified version or `prod` only for a system-wide `Int` type that is sufficient to compile `Base`. ``` julia prod(x::Tuple{}) = 1 # This is consistent with the regular prod because there is no need for size promotion # if all elements in the tuple are of system size. prod(x::Tuple{Int, Vararg{Int}}) = *(x...) ``` Although the implementations are different, they lead to the same binary code for tuples containing ~~`UInt` and~~ `Int`. ``` julia julia> a = (1,2,3) (1, 2, 3) # Simplified version for tuples containing Int only julia> prod_simplified(x::Tuple{Int, Vararg{Int}}) = *(x...) julia> @code_native prod_simplified(a) .text ; ┌ @ REPL[1]:1 within `prod_simplified' ; │┌ @ operators.jl:560 within `*' @ int.jl:88 movq 8(%rdi), %rax imulq (%rdi), %rax imulq 16(%rdi), %rax ; │└ retq nop ; └ ``` ``` julia # Regular prod without the simplification julia> @code_native prod(a) .text ; ┌ @ reduce.jl:588 within `prod` ; │┌ @ reduce.jl:588 within `#prod#247` ; ││┌ @ reduce.jl:289 within `mapreduce` ; │││┌ @ reduce.jl:289 within `#mapreduce#240` ; ││││┌ @ reduce.jl:162 within `mapfoldl` ; │││││┌ @ reduce.jl:162 within `#mapfoldl#236` ; ││││││┌ @ reduce.jl:44 within `mapfoldl_impl` ; │││││││┌ @ reduce.jl:48 within `foldl_impl` ; ││││││││┌ @ tuple.jl:276 within `_foldl_impl` ; │││││││││┌ @ operators.jl:613 within `afoldl` ; ││││││││││┌ @ reduce.jl:81 within `BottomRF` ; │││││││││││┌ @ reduce.jl:38 within `mul_prod` ; ││││││││││││┌ @ int.jl:88 within `*` movq 8(%rdi), %rax imulq (%rdi), %rax ; │││││││││└└└└ ; │││││││││┌ @ operators.jl:614 within `afoldl` ; ││││││││││┌ @ reduce.jl:81 within `BottomRF` ; │││││││││││┌ @ reduce.jl:38 within `mul_prod` ; ││││││││││││┌ @ int.jl:88 within `*` imulq 16(%rdi), %rax ; │└└└└└└└└└└└└ retq nop ; └ ```

Fix sum() and prod() for tuples

1c2e59c

petvana marked this pull request as ready for review July 8, 2021 14:00

JeffBezanson reviewed Jul 8, 2021

View reviewed changes

Reduce number of _prod_simple() occurrences

9476513

This comment has been minimized.

Sign in to view

petvana added 3 commits July 27, 2021 11:59

Merge branch 'master' into fix-tuple-prod-sum

3e4c90b

Reduce number of _prod_simple() occurrences thanks to JuliaLang#41201

69ee209

Remove unnecessary _prod_simple function

f834cdb

petvana marked this pull request as draft August 15, 2021 10:35

petvana marked this pull request as ready for review August 15, 2021 12:08

petvana requested a review from JeffBezanson August 15, 2021 12:08

NHDaly approved these changes Aug 26, 2021

View reviewed changes

base/tuple.jl Outdated Show resolved Hide resolved

test/tuple.jl Outdated Show resolved Hide resolved

test/tuple.jl Show resolved Hide resolved

petvana and others added 2 commits August 26, 2021 23:08

Apply suggestions from code review

5f3727c

Co-authored-by: Nathan Daly <[email protected]>

Merge branch 'JuliaLang:master' into fix-tuple-prod-sum

629f7ea

NHDaly approved these changes Sep 2, 2021

View reviewed changes

NHDaly merged commit bada80c into JuliaLang:master Sep 2, 2021

NHDaly mentioned this pull request Sep 2, 2021

prod for tuples uses * #39183

Closed

NHDaly linked an issue Sep 2, 2021 that may be closed by this pull request

prod for tuples uses * #39183

Closed

NHDaly added bug Indicates an unexpected problem or unintended behavior bugfix This change fixes an existing bug backport 1.7 and removed bug Indicates an unexpected problem or unintended behavior labels Sep 2, 2021

NHDaly added the backport 1.6 Change should be backported to release-1.6 label Sep 2, 2021

KristofferC mentioned this pull request Sep 2, 2021

release-1.7: Backports for 1.7-RC1 #41781

Merged

63 tasks

KristofferC mentioned this pull request Sep 3, 2021

Backports for julia-1.6.3 #41554

Merged

75 tasks

KristofferC removed the backport 1.6 Change should be backported to release-1.6 label Sep 6, 2021

KristofferC removed the backport 1.7 label Sep 8, 2021

lassepe mentioned this pull request Jun 1, 2022

Updates for julia 1.8 forrestlaine/TensorGames.jl#9

Merged

dpinol mentioned this pull request Jul 5, 2022

Make (again) length type match index type in sparse vectors #45937

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `sum()` and `prod()` for tuples #41510

Fix `sum()` and `prod()` for tuples #41510

petvana commented Jul 8, 2021 •

edited

Loading

JeffBezanson Jul 8, 2021

petvana Jul 8, 2021

petvana Aug 15, 2021

JeffBezanson commented Jul 8, 2021

This comment has been minimized.

petvana commented Aug 25, 2021

NHDaly left a comment

NHDaly commented Aug 26, 2021

petvana commented Aug 26, 2021

petvana commented Sep 2, 2021

NHDaly left a comment

NHDaly commented Sep 2, 2021

petvana commented Sep 2, 2021

KristofferC commented Sep 6, 2021

petvana commented Sep 6, 2021

KristofferC commented Sep 8, 2021

NHDaly commented Sep 26, 2021

Fix sum() and prod() for tuples #41510

Fix sum() and prod() for tuples #41510

Conversation

petvana commented Jul 8, 2021 • edited Loading

JeffBezanson Jul 8, 2021

Choose a reason for hiding this comment

petvana Jul 8, 2021

Choose a reason for hiding this comment

petvana Aug 15, 2021

Choose a reason for hiding this comment

JeffBezanson commented Jul 8, 2021

This comment has been minimized.

petvana commented Aug 25, 2021

NHDaly left a comment

Choose a reason for hiding this comment

NHDaly commented Aug 26, 2021

petvana commented Aug 26, 2021

petvana commented Sep 2, 2021

NHDaly left a comment

Choose a reason for hiding this comment

NHDaly commented Sep 2, 2021

petvana commented Sep 2, 2021

KristofferC commented Sep 6, 2021

petvana commented Sep 6, 2021

KristofferC commented Sep 8, 2021

NHDaly commented Sep 26, 2021

Fix `sum()` and `prod()` for tuples #41510

Fix `sum()` and `prod()` for tuples #41510

petvana commented Jul 8, 2021 •

edited

Loading