-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Functional alias to Base.Cartesian.@nif
#55093
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: Martin Holters <[email protected]>
Is this meant for users or for internal use? That is, is the idea to make it |
I'm not sure. Currently, all of Base.Cartesian is not public: https://docs.julialang.org/en/v1/devdocs/cartesian/. However, So, I'm not sure what to do. Any advice? Should Though regardless of
Will do |
It seems like the compiler is too-often choosing the non-generated part of the function during testing... Which is breaking the inference test I wrote because the non- Any way to force I could also remove the non- |
Co-authored-by: Martin Holters <[email protected]>
Hm. It still seems to take the non- |
Ok, I got it. The fix was -@inline function nif(condition, expression, ::Val{N}) where {N}
+@inline function nif(condition::F, expression::G, ::Val{N}) where {F,G,N}
nif(condition, expression, expression, Val(N))
end |
No clue why the test is failing:
that test works fine on my machine if I type it in the REPL. Could it be from the |
Base.Cartesian.@nif
with functional version Base.Cartesian.nif
Base.Cartesian.@nif
Co-authored-by: Martin Holters <[email protected]>
Argh. This seems to hurt inference quite a bit compared to using a normal Perhaps my functions are too complicated and the compiler avoids inlining: I am surprised because |
Replacing the loop with recursion could fix the bad inference, I guess. |
It seems even if I remove that branch, and have |
You can use
ntuple(f, Val(n))
as an alias of@ntuple(n, f)
. The reason this is preferable is because@ntuple
requiresn
to be known at parse time, whilentuple(f, Val(n))
only requires this information at compile time. Therefore you can haven
depend on types of arguments when using the functional alias, avoiding the need for@generated
functions in package code.However, there is no analog of
Base.Cartesian.@nif
, so you need to use@generated
whenever you wish to have a type-dependent if statement.This PR adds this alias as well as a test suite. Fixes #55087.
Use-cases
Some of my code in DynamicExpressions.jl needs to use
@generated
functions which roughly look like this:There's no way to do this without a
@generated
function, becauseN
needs to be known to the macro at parse time. The code otherwise does not need to use@generated
, and so likely incurs greater compilation effort than is actually needed.The reason I need this if statement is so that the compiler is aware of exactly which operator is used at each branch – required for things for Enzyme to work. (If I just used
getindex(t::Tuple, i::Int)
, the compiler would not be able to do constant propagation far enough).So, rather than require users and package developers to use a
@generated
whenever they want to create a type-dependent@nif
, this PR createsBase.Cartesian.nif
which does the code generation internally, in an analogous way to howntuple
works. This is also nice because it has the non-@generated
branch for improved inference in cases where the types are not fully known.For a simple example, consider the following test (included in the test suite):
Normally, had we used getindex here, inference would have not been able to infer that the return type never includes the first element. But since we used an
nif
, the compiler knows all possible branches, and can inlined + 1
, and therefore infer the correct type of every branch:To solve this problem with
Base.Cartesian.@nif
, you would have to wrap the function body with@generated
.Tests
Base.Cartesian.@nif
does not have any tests aside from thejldoctest
. This PR also adds many tests ofnif
which therefore also test@nif
.