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

[RFC] Add depwarn in constructor of Fixed{T,f} where f == 8sizeof(T) #159

Merged
merged 1 commit into from
Jan 11, 2020

Conversation

kimikage
Copy link
Collaborator

@kimikage kimikage commented Jan 2, 2020

If we officially deprecate Fixed{Int8, 8} and so on, it is better to add a depwarn early. (cf. #155)
However, I also think it is another possible option to continue to support them.

This also adds the domain checks for f in the constructors. Because of the dead code elimination, there is no performance regression for valid f in most use cases.

@kimikage kimikage changed the title Add depwarn in constructor of Fixed{T,f} where f == 8sizeof(T) [RFC] Add depwarn in constructor of Fixed{T,f} where f == 8sizeof(T) Jan 4, 2020
Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

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

👍 A small suggestion to print the requested values in the warning/error message. I wrote it out in just one place, but if you like it consider adding it to the DomainError messages too.

src/fixed.jl Outdated
Fixed{T, f}(i::Integer, _) where {T,f} = new{T, f}(i % T)
function Fixed{T, f}(i::Integer, _) where {T, f}
if f == bitwidth(T)
Base.depwarn("The `f`, which is the same as the number of rawtype bits, will be denied in the future.", :Fixed)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Base.depwarn("The `f`, which is the same as the number of rawtype bits, will be denied in the future.", :Fixed)
Base.depwarn("`Fixed` reserves one bit for the sign. Support for `f=$f` with raw type `T=$T` will be removed in a future release.", :Fixed)

To avoid a performance hit from the interpolation you might need the @noinline trick, i.e.,

@noinline dowarn(f, T) = Base.depwarn(...)
f == bitwidth(T) && dowarn(f, T)

(Probably not necessary if DCE runs before the allocation pass, I'm not sure of the order so you can check the LLVM code if you want to be sure.)

This also adds the domain checks for `f` in the constructors.
@kimikage
Copy link
Collaborator Author

I changed the messages.
The interpolation costs about 10% extra time. However, it is just a drop in the bucket, because depwarn increases processing time by about 2,000,000%.:sweat_smile:

@timholy
Copy link
Member

timholy commented Jan 10, 2020

But do you pay it even when it doesn't issue the warning? For example, check the LLVM for Q0f7(0.2) with the warning in the source and commented out. As long as it does DCE before the alloca pass it should be fine.

@kimikage
Copy link
Collaborator Author

kimikage commented Jan 11, 2020

"2,000,000%" means DCE works fine. I checked @code_llvm Q0f7(Int8(25), 0).
However, it is not a bad idea to check @code_llvm Q0f7(0.2) as a "memorial".

Edit:
Adding depwarn to the constructor of such a low-level data type means an "ultimatum".
Unless @timholy gives any additional instructions, I will merge this just for subsequent PRs. If anybody has another opinion, please let us know.

@kimikage
Copy link
Collaborator Author

I have to submit some PRs and merge them, so there is still time to release v0.8.0. For now, let's step forward.

@kimikage kimikage merged commit ca6e304 into JuliaMath:master Jan 11, 2020
@kimikage kimikage deleted the limit_f branch January 11, 2020 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants