Skip to content

remove Expr typ field #27499

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
Merged
merged 4 commits into from
Jun 12, 2018
Merged

remove Expr typ field #27499

merged 4 commits into from
Jun 12, 2018

Conversation

JeffBezanson
Copy link
Member

A long time coming! (see #24027)

We now keep type information in a separate array in the IR, so this field is redundant. Also Exprs are mostly used for surface ASTs where the typ field is not used at all, so it doesn't really make sense to have it.

@JeffBezanson JeffBezanson added breaking This change will break code compiler:codegen Generation of LLVM IR and native code compiler:inference Type inference compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) labels Jun 8, 2018
vtjnash
vtjnash reviewed Jun 8, 2018
View reviewed changes
base/compiler/abstractinterpretation.jl
# convert an inferred static parameter value to the inferred type of a static_parameter expression
function sparam_type(@nospecialize(val))
if isa(val, TypeVar)
if Any <: val.ub
Copy link
Member

Choose a reason for hiding this comment

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

val.ub may be a TypeVar

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR only moved this code. But also I think vars in bounds are normalized away in inferencestate.jl.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, OK – that makes sense. I was wondering what all of those loops were doing there.

base/compiler/ssair/inlining2.jl Outdated
@@ -644,9 +645,9 @@ function analyze_method!(idx, f, ft, metharg, methsp, method, stmt, atypes, sv,

@timeit "inline IR inflation" if src.codelocs === nothing
# TODO: another way to detect IR that uses slots?
ir2 = just_construct_ssa(src, ast, na-1)
ir2 = just_construct_ssa(src, ast, na-1), spvals_from_meth_instance(linfo)
Copy link
Member

Choose a reason for hiding this comment

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

clearly deadcode (old IR)

base/compiler/ssair/show.jl Outdated
end
#if print_typ && stmt.typ !== Any
# Base.print(io, "::$(stmt.typ)")
#end
Copy link
Member

Choose a reason for hiding this comment

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

delete?

base/compiler/ssair/show.jl Outdated
end
#if print_typ && stmt.typ !== Any
# Base.print(io, "::$(stmt.typ)")
#end
Copy link
Member

Choose a reason for hiding this comment

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

delete?

base/compiler/ssair/slot2ssa.jl
if x.head === :static_parameter
return sparam_type(spvals[x.args[1]])
elseif x.head === :boundscheck
return Bool
Copy link
Member

Choose a reason for hiding this comment

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

why are these cases necessary? It seems like it would be very bad if ci.ssavaluetypes[idx] didn't also have the right answer

Copy link
Member Author

Choose a reason for hiding this comment

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

These can occur as arguments to calls.

base/show.jl Outdated
show_type = false
end
#end
Copy link
Member

Choose a reason for hiding this comment

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

fixme?

test/compiler/compiler.jl Outdated
end
test_inferred_static(code)
#test_inferred_static(codetype)
Copy link
Member

Choose a reason for hiding this comment

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

fixme?

vtjnash
vtjnash approved these changes Jun 11, 2018
View reviewed changes
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

@nanosoldier runbenchmarks(ALL, vs = ":master")

@vtjnash
Copy link
Member

vtjnash commented Jun 11, 2018

@nanosoldier runbenchmarks(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

@ararslan
Copy link
Member

ararslan commented Jun 11, 2018 •
edited
Loading

atan2 needs its deprecations fixed in BaseBenchmarks; that's likely the cause of the atan2 slowdowns here.

Also, hooray, Nanosoldier lives again.

@vtjnash
Copy link
Member

vtjnash commented Jun 11, 2018

Looks good, except for maybe ["string", "join"]?

@ararslan
Copy link
Member

@nanosoldier runbenchmarks("string", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - no performance regressions were detected. A full report can be found here. cc @ararslan

@vtjnash
Copy link
Member

vtjnash commented Jun 12, 2018 •
edited
Loading

Alright. In that case - let’s merge!

@JeffBezanson JeffBezanson merged commit 83ce7ba into master Jun 12, 2018
@JeffBezanson JeffBezanson deleted the jb/exprtype2 branch June 12, 2018 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtjnash vtjnash vtjnash approved these changes

Assignees
No one assigned
Labels
breaking This change will break code compiler:codegen Generation of LLVM IR and native code compiler:inference Type inference compiler:optimizer Optimization passes (mostly in base/compiler/ssair/)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JeffBezanson @vtjnash @nanosoldier @ararslan