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

Using Base.@pure in ArrayInterface causes 265 problems #115

Closed
chriselrod opened this issue Feb 3, 2021 · 9 comments · Fixed by #119
Closed

Using Base.@pure in ArrayInterface causes 265 problems #115

chriselrod opened this issue Feb 3, 2021 · 9 comments · Fixed by #119

Comments

@chriselrod
Copy link
Collaborator

JuliaLang/julia#265 (comment)

I think we should err on the side of finding any other way.

chriselrod added a commit that referenced this issue Feb 3, 2021
@Tokazama
Copy link
Member

Tokazama commented Feb 4, 2021

In general, I agree that this should be avoided.
However, I think there are circumstances where it's the best tool we have especially when we are working purely with type information.
I don't recall defining a function with @pure that has multiple methods, but I'll double check everything.
Although I've tried to be cautious with my use of @pure by sticking to things found in Base or StaticArrays, I probably need to check for the presence of generic methods.
I've seen plenty of dot accessors and for loops (which I think ultimately call getproperty and iterate) used in @pure in Base, but I'll try to get rid of those too just in case.

@chriselrod
Copy link
Collaborator Author

This goes for @generated as well it seems, but one reason to avoid these is that unless you know compiler internals yourself to debug issues like 265, using Base.@pure is a good way to make sure you won't get any of the limited time of the developers who do.

@chriselrod
Copy link
Collaborator Author

FWIW, I posted an example on issue 265 where:

sirs() = simd_integer_register_size()
getoffset() = 4sirs()*2 + 2

sirs got updated, but getoffset did not:

julia> using VectorizationBase

julia> VectorizationBase.has_feature(::Val{:x86_64_avx2}) = VectorizationBase.False()

julia> using VectorizedRNG
[ Info: Precompiling VectorizedRNG [33b4df10-0173-11e9-2a0c-851a7edac40e]

julia> VectorizedRNG.getoffset()
514

julia> @code_warntype VectorizedRNG.getoffset()
MethodInstance for VectorizedRNG.getoffset()
  from getoffset() in VectorizedRNG at /home/chriselrod/.julia/dev/VectorizedRNG/src/xoshiro.jl:98
Arguments
  #self#::Core.Const(VectorizedRNG.getoffset)
Body::Int64
1%1 = VectorizedRNG.sirs()::Core.Const(static(16))
│   %2 = (4 * %1)::Core.Const(64)
│   %3 = (%2 * 2)::Core.Const(128)
│   %4 = (%3 + 2)::Core.Const(130)
└──      return %4


julia> VectorizedRNG.sirs()
static(16)

julia> VectorizedRNG.getoffset()
514

There was no @pure or @generated in between sirs() and getoffset().
But I think these are viewed as contaminating?

At this point, I'm losing motivation to address the issue myself.
Starting with Julia 1.6, LoopVectorizaton should get bad performance whenever the hardware info is wrong.
Before that, there was a risk it'd crash.

@Tokazama
Copy link
Member

Tokazama commented Feb 4, 2021

If I can find some time today I'll take a look at this. Maybe an extra pair of eyes will catch something.

@Tokazama
Copy link
Member

Tokazama commented Feb 4, 2021

There was no @pure or @generated in between sirs() and getoffset().

Does this mean that @pure and @generated are causing this particular issue?

@chriselrod
Copy link
Collaborator Author

There was none in between sirs(), which successfully updated, and getoffset() , which did not.
But I am not sure. I spent some time trying to get a smaller reproducer, but didn't succeed.

I'd like to get a reproducer free of any @generated or @pure anywhere (assuming it's possible) so that we can file a Julia issue that is harder to dismiss.

@Tokazama
Copy link
Member

Tokazama commented Feb 8, 2021

I think I solve a lot of the stuff I'm responsible for in #119. I'm also going to try to make more generated methods rely on Base.Cartesian in the future here, so that if they break things it's not our fault.

@ChrisRackauckas , I'm not sure what to do about this line here. I think type names are mutable, but I'm also not sure in what context this would actually change.
https://github.com/SciML/ArrayInterface.jl/blob/db2cfb44b2fa960727c3dc23b3f6ca087ccb6ecf/src/ArrayInterface.jl#L10

@ChrisRackauckas
Copy link
Member

I think we want to force that one, otherwise yes it won't infer. This is something no one should ever be mutating?

@chriselrod
Copy link
Collaborator Author

If we want to change it for the sake of not using Base.@pure, we could use @generated instead:

@generated __parameterless_type(::Type{T}) where {T} = Base.typename(T).wrapper

Of course, the same criticism of @pure there would apply to @generated, so changing it would be more political than practical AFAIK.

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 a pull request may close this issue.

3 participants