Skip to content

Simplify getfield and isdefined tfuncs by treating DataType as immutable#37293

Closed
martinholters wants to merge 2 commits intomasterfrom
mh/getfieldtfunc-datatype
Closed

Simplify getfield and isdefined tfuncs by treating DataType as immutable#37293
martinholters wants to merge 2 commits intomasterfrom
mh/getfieldtfunc-datatype

Conversation

@martinholters
Copy link
Member

And a little cleanup: Neither UnionAll nor SimpleVector should need special-casing. The former is immutable and gets handled as well as any other immutable, the latter has no fields.

Technically, DataType can be mutated, but actually doing that is looking for more trouble than just wrong constant propagation, and we've been doing this for some fields of it anyway. But maybe we can throw in the setfield builtin when trying to modify a DataType? Or is it even possible to declare DataType immutable?

elseif isa(arg1, Const)
arg1v = (arg1::Const).val
if !ismutable(arg1v) || isdefined(arg1v, idx) || (isa(arg1v, DataType) && is_dt_const_field(idx))
if !ismutable(arg1v) || isdefined(arg1v, idx) || isa(arg1v, DataType)
Copy link
Member

Choose a reason for hiding this comment

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

The types field can be undefined and then get set later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah. That's one of those where is_dt_const_field returned true, though.
Could we by any chance just trigger setting it here?

Copy link
Member

Choose a reason for hiding this comment

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

It can also get cleared later

Copy link
Member Author

Choose a reason for hiding this comment

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

So you're saying that

julia> foo() = isdefined(VersionNumber, :types)
foo (generic function with 1 method)

julia> @code_typed foo()
CodeInfo(
1return true
) => Bool

is bad because foo won't get recompiled when VersionNumber.types gets cleared and foo() will then wrongly still return true? That is bad, of course, but nothing this PR introduces. It's the same on 1.0, 1.5, and master. I'm not sure how big an undertaking fixing that would be, considering that some code (e.g. fieldcount) might rely on this (and getfield) being const-inferred.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I managed to construct a test case for the "types may get set later" case:

g37293(::Type{T}) where T = isdefined(T, :types)
struct T37293; x::Val{g37293(T37293)}; end # call g37293 on incomplete type with not-yet-set types
@test g37293(T37293) == isdefined(T37293, :types)

This fails on master, but I have an update to this PR in the working that will fix it.

@vtjnash how can I provoke clearing the types field?

@martinholters
Copy link
Member Author

While at it, what about the fields of TypeName? Currently we treat only name, module, and wrapper as constant. These look ok? Is names constant? That would be helpful for fieldcount now that (::DataType).types cannot be treated as constant anymore. What about hash?

@martinholters martinholters force-pushed the mh/getfieldtfunc-datatype branch from 1e1d6ae to d690166 Compare September 1, 2020 10:19
@JeffBezanson
Copy link
Member

I believe TypeName.names is either constant or undefined.

@martinholters
Copy link
Member Author

I believe TypeName.names is either constant or undefined.

But it can become set later if undefined now? So if undefined infer getfield as SimpleVector (not Bottom), if set to n, infer as Const(n) would be ok?

Coming back to (::DataType).types once more. Even if it can get cleared, as long as it cannot be set to a different value than before, infering getfield on it as Const (if not undefined) would still be ok, as that does not automatically imply nothrow. Would that be valid? (I've tried figuring out how/when types gets populated, but I couldn't even figure out where it would be cleared in a way visible to Julia code.)

So to summarize:

field status isdefined getfield remarks
(::DataType).types undef Bool SimpleVector
set Bool Const assuming can get cleared, but not be set differently
(::TypeName).names undef Bool SimpleVector assuming can be set
set Const(true) Const ...but neither cleared nor set differently

Does that look ok? Then I'll implement it in this PR.

@martinholters martinholters force-pushed the mh/getfieldtfunc-datatype branch from d690166 to b38686a Compare September 8, 2020 14:57
@vtjnash vtjnash closed this Mar 28, 2022
@vtjnash vtjnash deleted the mh/getfieldtfunc-datatype branch March 28, 2022 20:48
@vtjnash
Copy link
Member

vtjnash commented Mar 28, 2022

Replaced by better effects modeling and const fields of mutables

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

compiler:inference Type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants