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.

Sign up for GitHub

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

Jump to bottom

Fix justActive condition #1936

Merged
merged 1 commit into from
Oct 22, 2024
+12 −1
Merged

Fix justActive condition #1936

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/compiler.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,7 @@ end

subT = fieldtype(T, f)

if justActive && !allocatedinline(subT)
if justActive && ismutabletype(subT)
Copy link
Member

Choose a reason for hiding this comment

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

So I understand the failure but I’m confused why this would resolve it.

@vchuravy do you have cycles to review as well

Copy link
Contributor Author

@danielwe danielwe Oct 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

As mentioned in #1935 (comment), the problem is that a struct that contains non-isbits fields and can be incompletely initialized will not be allocated inline, even if it is immutable. As such, it's an error to conclude anything about activity from allocatedinline. A type can have immutable semantics and thus contain active values even if it is not allocated inline.

A mutable type, however, is of either duplicated or const type, hence it's safe to take this early return path. (Judging by #1926 this is true even if the mutable type has differentiable const fields.)

Copy link
Contributor Author

@danielwe danielwe Oct 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I suppose since a non-inlined value necessarily has a memory address, an alternative would be to disregard the purported immutability and let make_zero! zero out the relevant fields anyway. However, the ccall(:jl_set_nth_field, ...) solution from #1926 doesn't work on these types, so it would have to be done with raw pointer twiddling.

But this doesn't sound appealing to me for several reasons, the most important of which is that you can now get a DupState type by nesting an immutable ActiveState type inside an immutable container (provided the justActive = false case is changed to be consistent with this choice). See Tuple{Incomplete} from the test case for an example; in other words, this would require a user to annotate an Incomplete argument as Active or MixedDuplicated, but a Tuple{Incomplete} argument as Duplicated. This would be non-intuitive, hard to reason about, and probably even harder to explain to users. Better to respect that Incomplete is immutable as declared. That leaves us with the above conclusion: you can't use allocatedinline to conclude about activity state.

return Val(AnyState)
end

Expand Down
11 changes: 11 additions & 0 deletions test/runtests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,17 @@ end
@test Enzyme.Compiler.active_reg_inner(Tuple, (), nothing) == Enzyme.Compiler.DupState
@test Enzyme.Compiler.active_reg_inner(Tuple, (), nothing, #=justactive=#Val(false), #=unionsret=#Val(false), #=abstractismixed=#Val(true)) == Enzyme.Compiler.MixedState
@test Enzyme.Compiler.active_reg_inner(Tuple{A,A} where A, (), nothing, #=justactive=#Val(false), #=unionsret=#Val(false), #=abstractismixed=#Val(true)) == Enzyme.Compiler.MixedState

# issue #1935
struct Incomplete
x::Float64
y
Incomplete(x) = new(x)
# incomplete constructor & non-bitstype field => !Base.allocatedinline(Incomplete)
end
@test Enzyme.Compiler.active_reg_inner(Tuple{Incomplete}, (), nothing, #=justActive=#Val(false)) == Enzyme.Compiler.MixedState
@test Enzyme.Compiler.active_reg_inner(Tuple{Incomplete}, (), nothing, #=justActive=#Val(true)) == Enzyme.Compiler.ActiveState

world = codegen_world_age(typeof(f0), Tuple{Float64})
thunk_a = Enzyme.Compiler.thunk(Val(world), Const{typeof(f0)}, Active, Tuple{Active{Float64}}, Val(API.DEM_ReverseModeCombined), Val(1), Val((false, false)), Val(false), Val(false), DefaultABI, Val(false), Val(false))
thunk_b = Enzyme.Compiler.thunk(Val(world), Const{typeof(f0)}, Const, Tuple{Const{Float64}}, Val(API.DEM_ReverseModeCombined), Val(1), Val((false, false)), Val(false), Val(false), DefaultABI, Val(false), Val(false))
Expand Down
Loading