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

Compiler: avoid creating Set in merge_if_vars #12433

Merged
merged 2 commits into from
Sep 2, 2022
Merged

Compiler: avoid creating Set in merge_if_vars #12433

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f3571e3
Compiler: avoid creating Set in merge_if_vars
Aug 31, 2022
7334c77
Suggestion by @straight-shoota
Aug 31, 2022
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
134 changes: 68 additions & 66 deletions src/compiler/crystal/semantic/main_visitor.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1887,87 +1887,89 @@ module Crystal
# - Don't use the type of a branch that is unreachable (ends with return,
# break or with a call that is NoReturn)
def merge_if_vars(node, cond_vars, then_vars, else_vars, before_then_vars, before_else_vars, then_unreachable, else_unreachable)
all_vars_names = Set(String).new
then_vars.each_key do |name|
all_vars_names << name
end
else_vars.each_key do |name|
all_vars_names << name
then_vars.each do |name, then_var|
else_var = else_vars[name]?

merge_if_var(name, node, cond_vars, then_var, else_var, before_then_vars, before_else_vars, then_unreachable, else_unreachable)
end

all_vars_names.each do |name|
else_vars.each do |name, else_var|
then_var = then_vars[name]?
else_var = else_vars[name]?
next if then_var

# Check whether the var didn't change at all
next if then_var.same?(else_var)
merge_if_var(name, node, cond_vars, then_var, else_var, before_then_vars, before_else_vars, then_unreachable, else_unreachable)
Copy link
Member

Choose a reason for hiding this comment

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

then_var must be nil after next. So I'm wondering if this tiny refactor might be a bit better? It just avoids fetching the actual then_var, because the value is not used.

Suggested change
then_var = then_vars[name]?
else_var = else_vars[name]?
next if then_var
# Check whether the var didn't change at all
next if then_var.same?(else_var)
merge_if_var(name, node, cond_vars, then_var, else_var, before_then_vars, before_else_vars, then_unreachable, else_unreachable)
next if then_vars.has_key?(name)
merge_if_var(name, node, cond_vars, nil, else_var, before_then_vars, before_else_vars, then_unreachable, else_unreachable)

Copy link
Member

Choose a reason for hiding this comment

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

The diff looks very weird. I don't know why... Just ignore the first 5 deleted lines in the suggestion.

end
end

cond_var = cond_vars[name]?
def merge_if_var(name, node, cond_vars, then_var, else_var, before_then_vars, before_else_vars, then_unreachable, else_unreachable)
# Check whether the var didn't change at all
return if then_var.same?(else_var)

# Only copy `nil_if_read` from each branch if it's not unreachable
then_var_nil_if_read = !then_unreachable && then_var.try(&.nil_if_read?)
else_var_nil_if_read = !else_unreachable && else_var.try(&.nil_if_read?)
if_var_nil_if_read = !!(then_var_nil_if_read || else_var_nil_if_read)
cond_var = cond_vars[name]?

# Check if no types were changes in either then 'then' and 'else' branches
if cond_var && !then_unreachable && !else_unreachable
if then_var.same?(before_then_vars[name]?) &&
else_var.same?(before_else_vars[name]?)
cond_var.nil_if_read = if_var_nil_if_read
@vars[name] = cond_var
next
end
end
# Only copy `nil_if_read` from each branch if it's not unreachable
then_var_nil_if_read = !then_unreachable && then_var.try(&.nil_if_read?)
else_var_nil_if_read = !else_unreachable && else_var.try(&.nil_if_read?)
if_var_nil_if_read = !!(then_var_nil_if_read || else_var_nil_if_read)

if_var = MetaVar.new(name)
if_var.nil_if_read = if_var_nil_if_read
# Check if no types were changes in either then 'then' and 'else' branches
if cond_var && !then_unreachable && !else_unreachable
if then_var.same?(before_then_vars[name]?) &&
else_var.same?(before_else_vars[name]?)
cond_var.nil_if_read = if_var_nil_if_read
@vars[name] = cond_var
return
end
end

if then_var && else_var
if then_unreachable
if_var.bind_to conditional_no_return(node.then, then_var)
else
if_var.bind_to then_var
end
if_var = MetaVar.new(name)
if_var.nil_if_read = if_var_nil_if_read

if else_unreachable
if_var.bind_to conditional_no_return(node.else, else_var)
else
if_var.bind_to else_var
end
elsif then_var
if then_unreachable
if_var.bind_to conditional_no_return(node.then, then_var)
else
if_var.bind_to then_var
end
if then_var && else_var
if then_unreachable
if_var.bind_to conditional_no_return(node.then, then_var)
else
if_var.bind_to then_var
end

if cond_var
if_var.bind_to cond_var
elsif !else_unreachable
if_var.bind_to program.nil_var
if_var.nil_if_read = true
else
if_var.bind_to conditional_no_return(node.else, @program.nil_var)
end
elsif else_var
if else_unreachable
if_var.bind_to conditional_no_return(node.else, else_var)
else
if_var.bind_to else_var
end
if else_unreachable
if_var.bind_to conditional_no_return(node.else, else_var)
else
if_var.bind_to else_var
end
elsif then_var
if then_unreachable
if_var.bind_to conditional_no_return(node.then, then_var)
else
if_var.bind_to then_var
end

if cond_var
if_var.bind_to cond_var
elsif !then_unreachable
if_var.bind_to program.nil_var
if_var.nil_if_read = true
else
if_var.bind_to conditional_no_return(node.then, @program.nil_var)
end
if cond_var
if_var.bind_to cond_var
elsif !else_unreachable
if_var.bind_to program.nil_var
if_var.nil_if_read = true
else
if_var.bind_to conditional_no_return(node.else, @program.nil_var)
end
elsif else_var
if else_unreachable
if_var.bind_to conditional_no_return(node.else, else_var)
else
if_var.bind_to else_var
end

@vars[name] = if_var
if cond_var
if_var.bind_to cond_var
elsif !then_unreachable
if_var.bind_to program.nil_var
if_var.nil_if_read = true
else
if_var.bind_to conditional_no_return(node.then, @program.nil_var)
end
end

@vars[name] = if_var
end

def conditional_no_return(node, var)
Expand Down