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

Erroneous test in julia/test /float16.jl? #56582

Closed
stensmo opened this issue Nov 17, 2024 · 3 comments · Fixed by #56630
Closed

Erroneous test in julia/test /float16.jl? #56582

stensmo opened this issue Nov 17, 2024 · 3 comments · Fixed by #56630
Labels
float16 system:riscv Support for Risc-V cpus

Comments

@stensmo
Copy link

stensmo commented Nov 17, 2024

I am testing Julia on RISC-V, and hit a small snag, but the test could perhaps be removed?

@test unsafe_trunc(Int16, NaN16) === Int16(0) #18771

Is on line 82. Julia will issue this code (Both on X86 and Risc-V)

define i16 @julia_unsafe_trunc_1513(half %"x::Float16") #0 {
top:
  %0 = fptosi half %"x::Float16" to i16
  %1 = freeze i16 %0
  ret i16 %1
}

When you check the LLVM Language Reference for fptosi https://llvm.org/docs/LangRef.html#fptosi-to-instruction you see that if the value does not fit, it's a poison value, and it's ok to replace the value with any value of that type.

On x86 the poison value is 0 and on Risc-V the posion value is -1 for this case. Why does Julia need the poison value to be 0?

@giordano
Copy link
Contributor

This test was added by @timholy in #21312.

@giordano giordano added float16 system:riscv Support for Risc-V cpus labels Nov 17, 2024
@giordano
Copy link
Contributor

I feel like the test may be a victim of Hyrum's Law, and checking unsafe_trunc(Int16, NaN16) isa Int16 would be enough.

@oscardssmith
Copy link
Member

yeah. the return value of this is very much not specified.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
float16 system:riscv Support for Risc-V cpus
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants