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.

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

fix: fixes issues #225 #232

Merged
merged 1 commit into from
Jun 23, 2020
Merged

Conversation

baszalmstra
Copy link
Collaborator

@baszalmstra baszalmstra commented Jun 19, 2020

Fixes an issue where taking the field of a temporary crashed the compiler. This was caused by a field expression always expecting a "place" expression which is actually not always the case (in the case of a temporary for example).

Closes #225

@codecov
Copy link

codecov bot commented Jun 19, 2020

Codecov Report

Merging #232 into master will increase coverage by 0.00%.
The diff coverage is 86.20%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #232   +/-   ##
=======================================
  Coverage   81.13%   81.14%           
=======================================
  Files         182      182           
  Lines       11938    11968   +30     
=======================================
+ Hits         9686     9711   +25     
- Misses       2252     2257    +5     
Impacted Files Coverage Δ
crates/mun_codegen/src/ir/body.rs 84.87% <85.96%> (-0.10%) ⬇️
crates/mun_codegen/src/test.rs 98.24% <100.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b738800...8078734. Read the comment docs.

@baszalmstra baszalmstra requested a review from Wodann June 19, 2020 17:50
Comment on lines +19 to +28
%init = insertvalue %Num undef, i64 %0, 0
%new_ptr = load i8** (i8*, i8*)*, i8** (i8*, i8*)** getelementptr inbounds (%DispatchTable, %DispatchTable* @dispatchTable, i32 0, i32 0)
%Num_ptr = load %struct.MunTypeInfo*, %struct.MunTypeInfo** getelementptr inbounds ([5 x %struct.MunTypeInfo*], [5 x %struct.MunTypeInfo*]* @global_type_table, i32 0, i32 2)
%type_info_ptr_to_i8_ptr = bitcast %struct.MunTypeInfo* %Num_ptr to i8*
%allocator_handle = load i8*, i8** @allocatorHandle
%new = call i8** %new_ptr(i8* %type_info_ptr_to_i8_ptr, i8* %allocator_handle)
%Num_ptr_ptr = bitcast i8** %new to %Num**
%Num_mem_ptr = load %Num*, %Num** %Num_ptr_ptr
store %Num %init, %Num* %Num_mem_ptr
%mem_ptr = load %Num*, %Num** %Num_ptr_ptr
Copy link
Collaborator

Choose a reason for hiding this comment

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

Future Work
This is a great example of where we can optimise a lot by using a struct(value) instead of a struct(gc). For temporaries (or scoped struct instances) it doesn't really make sense to allocate them on the heap.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would be great! But the use case is quite small, it's only when you construct a GC struct that doesn't leave the scope of the function at all (otherwise you can't make guarantees). Nonethess, if we can catch that in the compiler and turn it into a stack allocation, that would be great!

@baszalmstra baszalmstra changed the title fix: fixes issue #225 fix: fixes issues #225 and #227 Jun 20, 2020
@baszalmstra baszalmstra changed the title fix: fixes issues #225 and #227 fix: fixes issues #225 Jun 20, 2020
@baszalmstra baszalmstra merged commit e2a25f2 into mun-lang:master Jun 23, 2020
@Wodann Wodann added this to the Mun v0.3.0 milestone Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compiler panics when accessing a field of a temporary
2 participants