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

Slightly generalize _compute_sparam elision #48144

Merged
merged 1 commit into from
Jan 6, 2023
Merged

Slightly generalize _compute_sparam elision #48144

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
25 changes: 18 additions & 7 deletions base/compiler/ssair/passes.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -796,7 +796,6 @@ end
end && return nothing

arg = sig.parameters[i]
isa(arg, DataType) || return nothing

rarg = def.args[2 + i]
isa(rarg, SSAValue) || return nothing
Expand All @@ -805,6 +804,10 @@ end
rarg = argdef.args[1]
isa(rarg, SSAValue) || return nothing
argdef = compact[rarg][:inst]
else
isa(arg, DataType) || return nothing
isType(arg) || return nothing
Comment on lines +808 to +809
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
isa(arg, DataType) || return nothing
isType(arg) || return nothing
isType(arg) || return nothing

isType(x) includes isa(x, DataType).

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do in a follow up

arg = arg.parameters[1]
end

is_known_call(argdef, Core.apply_type, compact) || return nothing
Expand All @@ -815,15 +818,23 @@ end
applyT = applyT.val

isa(applyT, UnionAll) || return nothing
# N.B.: At the moment we only lift the valI == 1 case, so we
# only need to look at the outermost tvar.
applyTvar = applyT.var
applyTbody = applyT.body
Keno marked this conversation as resolved.
Show resolved Hide resolved

isa(applyTbody, DataType) || return nothing
applyTbody.name == arg.name || return nothing
length(applyTbody.parameters) == length(arg.parameters) == 1 || return nothing
applyTbody.parameters[1] === applyTvar || return nothing
arg.parameters[1] === tvar || return nothing
return LiftedValue(argdef.args[3])
arg = unwrap_unionall(arg)
applyTbody = unwrap_unionall(applyTbody)
Keno marked this conversation as resolved.
Show resolved Hide resolved

(isa(arg, DataType) && isa(applyTbody, DataType)) || return nothing
applyTbody.name === arg.name || return nothing
length(applyTbody.parameters) == length(arg.parameters) || return nothing
for i = 1:length(applyTbody.parameters)
if applyTbody.parameters[i] === applyTvar && arg.parameters[i] === tvar
return LiftedValue(argdef.args[3])
end
end
Comment on lines +831 to +836
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it it may be clearer (and more extensible later) if written this way:

Suggested change
length(applyTbody.parameters) == length(arg.parameters) || return nothing
for i = 1:length(applyTbody.parameters)
if applyTbody.parameters[i] === applyTvar && arg.parameters[i] === tvar
return LiftedValue(argdef.args[3])
end
end
# find a possible parameter number we are extracting when matching with `tvar`
i = findfirst(===(tvar), arg.parameters)
# determine if we can find the apply_type for that parameter
if applyTvar === applyTbody.parameters[i]
return LiftedValue(argdef.args[3])
end

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure whether we want to look at all the parameters or just the first one. We can probably look at the first one in applyTbody, but I think arg might potentially have multiple instances.

Copy link
Member

Choose a reason for hiding this comment

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

First is indeed probably the most common to extract

return nothing
end

# NOTE we use `IdSet{Int}` instead of `BitSet` for in these passes since they work on IR after inlining,
Expand Down
8 changes: 8 additions & 0 deletions test/compiler/irpasses.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1221,3 +1221,11 @@ function a47180(b; stdout )
c
end
@test isa(a47180(``; stdout), Base.AbstractCmd)

# Test that _compute_sparams can be eliminated for NamedTuple
named_tuple_elim(name::Symbol, result) = NamedTuple{(name,)}(result)
let src = code_typed1(named_tuple_elim, Tuple{Symbol, Tuple})
@test count(iscall((src, Core._compute_sparams)), src.code) == 0 &&
count(iscall((src, Core._svec_ref)), src.code) == 0 &&
count(iscall(x->!isa(argextype(x, src).val, Core.Builtin)), src.code) == 0
end