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

prod for tuples uses * #39183

Closed
yurivish opened this issue Jan 10, 2021 · 3 comments · Fixed by #41510
Closed

prod for tuples uses * #39183

yurivish opened this issue Jan 10, 2021 · 3 comments · Fixed by #41510
Labels
bug Indicates an unexpected problem or unintended behavior maths Mathematical functions stdlib Julia's standard library

Comments

@yurivish
Copy link
Contributor

julia> prod([Int8(100), Int8(100)])
10000

julia> prod((Int8(100), Int8(100)))
16

julia> versioninfo()
Julia Version 1.5.3
Commit 788b2c77c1 (2020-11-09 13:37 UTC)
Platform Info:
  OS: macOS (x86_64-apple-darwin18.7.0)
  CPU: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-9.0.1 (ORCJIT, skylake)

Ref. #39182, which is the same issue but for sum and +

The is a todo in the code that says this function definition is inconsistent with regular prod:

julia/base/tuple.jl

Lines 399 to 400 in 788b2c7

# TODO: this is inconsistent with the regular prod in cases where the arguments
# require size promotion to system size.

@yurivish
Copy link
Contributor Author

yurivish commented Jan 10, 2021

I just noticed that the sum and prod definitions are right next to each other in the code with the same comment on both, so it might not be worth having a separate issue. Would the fix be the same in both cases?

@NHDaly
Copy link
Member

NHDaly commented Jan 23, 2021

👍 according to the docstring, this is definitely a bug, yeah:

help?> prod([Int8(100), Int8(100)])
  prod(itr)

  Returns the product of all elements of a collection.

  The return type is Int for signed integers of less than system word size, and UInt for unsigned integers of less than system word size. For all other
  arguments, a common return type is found to which all arguments are promoted.

  Examples
  ≡≡≡≡≡≡≡≡≡≡

  julia> prod(1:20)
  2432902008176640000

@NHDaly NHDaly added bug Indicates an unexpected problem or unintended behavior maths Mathematical functions stdlib Julia's standard library labels Jan 23, 2021
NHDaly pushed a commit that referenced this issue Sep 2, 2021
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
; └
```
@NHDaly
Copy link
Member

NHDaly commented Sep 2, 2021

Closed in #41510.

@NHDaly NHDaly closed this as completed Sep 2, 2021
@NHDaly NHDaly linked a pull request Sep 2, 2021 that will close this issue
KristofferC pushed a commit that referenced this issue Sep 2, 2021
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 pushed a commit that referenced this issue Sep 3, 2021
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 pushed a commit that referenced this issue Sep 3, 2021
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)
LilithHafner pushed a commit to LilithHafner/julia that referenced this issue Feb 22, 2022
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
; └
```
LilithHafner pushed a commit to LilithHafner/julia that referenced this issue Mar 8, 2022
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
; └
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior maths Mathematical functions stdlib Julia's standard library
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants