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

Array fixes for non-power-of-2 sized elements #33283

Merged
merged 1 commit into from
Oct 1, 2019
Merged

Conversation

JeffBezanson
Copy link
Member

Before:

julia> primitive type TypeWith24Bits 24 end

julia> x = Core.Intrinsics.trunc_int(TypeWith24Bits, 0x112233); a = [x,x];

julia> Core.arrayset(true, a, x, 2)
2-element Array{TypeWith24Bits,1}:
 TypeWith24Bits(0x112233)
 TypeWith24Bits(0x111122)

After: works.

Fixes JuliaData/JuliaDB.jl#287 (comment)

@JeffBezanson JeffBezanson added arrays [a, r, r, a, y, s] bugfix This change fixes an existing bug labels Sep 16, 2019
test/core.jl Outdated
# non-power-of-2 element sizes
primitive type TypeWith24Bits 24 end
let x = Core.Intrinsics.trunc_int(TypeWith24Bits, 0x112233), a = [x,x]
Core.arrayset(true, a, x, 2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a[2] = x, or are you intentionally trying to avoid this being compiled (could use a comment)?

Can you add a test for a = [(x, x), (x, x)] also?

Might be good to use something other than x here also, so that the mutation here is observable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Yes, I was trying to avoid the compiled form since I believe it does the right thing.

@JeffBezanson JeffBezanson force-pushed the jb/nonpow2elt branch 3 times, most recently from ba2f6cc to e29fd4a Compare September 19, 2019 22:15
@chethega
Copy link
Contributor

Can we add the backport label?

Reason is that @code_native Base.elsize([]) is a real mess before (afaiu because Core.sizeof(Ptr) and Ptr is non-concrete, which messes up constant folding). Now we use Core.sizeof(Ptr{Cvoid}) which constant folds properly.

@JeffBezanson
Copy link
Member Author

In julia 1.x:

julia> @code_native Base.elsize([])
	.text
; Function elsize {
; Location: abstractarray.jl:106
	movl	$8, %eax
	retq
	nopw	%cs:(%rax,%rax)
;}

How to reproduce what you're seeing?

@chethega
Copy link
Contributor

chethega commented Oct 15, 2019

julia> versioninfo()
Julia Version 1.2.0
Commit c6da87ff4b (2019-08-20 00:03 UTC)
Platform Info:
  OS: Linux (x86_64-pc-linux-gnu)
  CPU: Intel(R) Core(TM) i5-5###U CPU @ 2.00GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-6.0.1 (ORCJIT, broadwell)

julia> @code_typed Base.elsize([])
CodeInfo(
1 ─ %1 = Base.Ptr::Type{Ptr}
│   %2 = Core.sizeof::typeof(Core.sizeof)
│   %3 = (%2)(%1)::Int64
└──      return %3
) => Int64

julia> @code_native Base.elsize([])
	.text
; ┌ @ abstractarray.jl:153 within `elsize' @ array.jl:201
; │┌ @ abstractarray.jl:153 within `sizeof'
	pushq	%rax
	movabsq	$140487341557840, %rax  # imm = 0x7FC5C216C850
	movq	%rax, (%rsp)
	movabsq	$jl_f_sizeof, %rax
	movq	%rsp, %rsi
	xorl	%edi, %edi
	movl	$1, %edx
	callq	*%rax
; │└
; │ @ abstractarray.jl:153 within `elsize'
	movq	(%rax), %rax
	popq	%rcx
	retq
	nopw	(%rax,%rax)

That is fixed on master. I saw the spurious jl_f_sizeof call in some pointer(a, i) and tracked the fix back to this PR. I have only tested master and 1.2.0, but git blame suggests that it is not a recent regression.

Core.sizeof(Ptr) is not constant-folded on master either, but your PR changed to Core.sizeof(Ptr{Cvoid}) (and also fixed the original issue, and also added @pure for good measure). A minimal backport would just replace Core.sizeof(Ptr) with Core.sizeof(Ptr{Cvoid}).

@KristofferC
Copy link
Member

julia> @code_native Base.elsize([])
        .text
; ┌ @ abstractarray.jl:153 within `elsize'
        movl    $8, %eax
        retq
        nopw    %cs:(%rax,%rax)
; └

on 1.3 at least.

@KristofferC KristofferC mentioned this pull request Nov 7, 2019
19 tasks
JeffBezanson added a commit that referenced this pull request Nov 7, 2019
JeffBezanson added a commit that referenced this pull request Nov 8, 2019
@KristofferC KristofferC mentioned this pull request Dec 3, 2019
56 tasks
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] bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EXCEPTION_ACCESS_VIOLATION working with UInt160
5 participants