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

Int(true) == 255 with broadcasting? #52127

Closed
MarcMush opened this issue Nov 11, 2023 · 4 comments · Fixed by #52189
Closed

Int(true) == 255 with broadcasting? #52127

MarcMush opened this issue Nov 11, 2023 · 4 comments · Fixed by #52189
Labels
backport 1.10 Change should be backported to the 1.10 release compiler:codegen Generation of LLVM IR and native code

Comments

@MarcMush
Copy link
Contributor

MarcMush commented Nov 11, 2023

only in 1.9.0-1.9.3, I get this strange bug (not 1.8, not 1.10.0-rc1)

julia> f(n)=(n-=1)<0||[f(n)]
f (generic function with 1 method)

julia> Int.(f.(0))
255

julia> a = f.(0)
true

julia> Int.(a)
1

julia> Int.(true)
1

It seems to happen only when f is non type-stable

@BioTurboNick
Copy link
Contributor

I reproduced your issue. If I provide a type annotation to f it works:

julia> f(n) = (n-=1) < 0 || [f(n)]
f (generic function with 1 method)

julia> Int.(f.(0))
255

julia> f(n)::Bool = (n-=1) < 0 || [f(n)]
f (generic function with 1 method)

julia> Int.(f.(0))
1

@BioTurboNick
Copy link
Contributor

Curious...

julia> bitstring(f.(0))
"00000001"

julia> bitstring.(f.(0))
"00001111"

@KristofferC KristofferC added needs tests Unit tests are required for this change bisect wanted labels Nov 13, 2023
@giordano
Copy link
Contributor

I haven't run git bisect, but

  • works on 6dae654 (commit before upgrade to LLVM 14)
  • doesn't work on a12c2f0 (commit of upgrade to LLVM 14)
  • doesn't work on b2273d3 (commit before upgrade to LLVM 15)
  • I couldn't build 190f841 (commit of upgrade to LLVM 15) because I'm getting compilation errors

Everything points to a bug in LLVM 14.

@vtjnash vtjnash removed needs tests Unit tests are required for this change bisect wanted labels Nov 15, 2023
@vtjnash
Copy link
Member

vtjnash commented Nov 15, 2023

Looks like our generated IR here is wrong. It is not legal to take this bitcast, since most of the bits are undef:

julia> @code_llvm optimize=false f(0)

L4:                                               ; preds = %top
  store i1 %8, i1* %2, align 1
  %12 = bitcast i1* %2 to i8*
  %13 = bitcast [1 x i8]* %0 to i8*
  switch i8 1, label %union_move_skip [
    i8 1, label %union_move
  ]

On some versions of LLVM, it takes the liberty to replace those undef bits with -1. On other versions, it takes those liberties to instead give up and not optimize the function instead.

(can't repro this on master exactly, but can confirm the generated IR is still wrong)

@giordano giordano added the compiler:codegen Generation of LLVM IR and native code label Nov 15, 2023
@vtjnash vtjnash added the backport 1.10 Change should be backported to the 1.10 release label Nov 15, 2023
vtjnash added a commit that referenced this issue Nov 16, 2023
Teach value_to_pointer to convert primitive types to their stored
representation first, to avoid exposing undef bits later (via memcpy).

Take this opportunity to also generalizes the support for zext Bool to
anywhere inside any struct for changing any bitwidth to a multiple of 8
bytes. This would change a vector like <2 x i4> from occupying i8 to i16
(c.f. LLVM's LangRef), if such an operation were expressible in Julia
today. And take this opportunity to do a bit of code cleanup, now that
codegen is better and using helpers from LLVM.

Fixes #52127
JeffBezanson pushed a commit that referenced this issue Nov 17, 2023
Teach value_to_pointer to convert primitive types to their stored
representation first, to avoid exposing undef bits later (via memcpy).

Take this opportunity to also generalizes the support for zext Bool to
anywhere inside any struct for changing any bitwidth to a multiple of 8
bytes. This would change a vector like <2 x i4> from occupying i8 to i16
(c.f. LLVM's LangRef), if such an operation were expressible in Julia
today. And take this opportunity to do a bit of code cleanup, now that
codegen is better and using helpers from LLVM.

Fixes #52127
KristofferC pushed a commit that referenced this issue Nov 27, 2023
Teach value_to_pointer to convert primitive types to their stored
representation first, to avoid exposing undef bits later (via memcpy).

Take this opportunity to also generalizes the support for zext Bool to
anywhere inside any struct for changing any bitwidth to a multiple of 8
bytes. This would change a vector like <2 x i4> from occupying i8 to i16
(c.f. LLVM's LangRef), if such an operation were expressible in Julia
today. And take this opportunity to do a bit of code cleanup, now that
codegen is better and using helpers from LLVM.

Fixes #52127

(cherry picked from commit 9aa7980)
KristofferC pushed a commit that referenced this issue Nov 27, 2023
Teach value_to_pointer to convert primitive types to their stored
representation first, to avoid exposing undef bits later (via memcpy).

Take this opportunity to also generalizes the support for zext Bool to
anywhere inside any struct for changing any bitwidth to a multiple of 8
bytes. This would change a vector like <2 x i4> from occupying i8 to i16
(c.f. LLVM's LangRef), if such an operation were expressible in Julia
today. And take this opportunity to do a bit of code cleanup, now that
codegen is better and using helpers from LLVM.

Fixes #52127

(cherry picked from commit 9aa7980)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.10 Change should be backported to the 1.10 release compiler:codegen Generation of LLVM IR and native code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants