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

[GCLowering] Fix phis of unions #26989

Merged
merged 5 commits into from
May 6, 2018
Merged

[GCLowering] Fix phis of unions #26989

merged 5 commits into from
May 6, 2018

Conversation

Keno
Copy link
Member

@Keno Keno commented May 4, 2018

This fixes gc lowering for phis of the form:

%phi = phi {%jl_value_t addrspace(10)*, i8} [%aunion, %a], [%bunion, %b]

This kind of struct (used by the tagged union representation) has special support
in GC lowering. After optimization, these structs often survive quite far as a
struct (i.e. the destructuring of them is often elided). We simply need to extend
this support to the cases that handle phi and select nodes.

Fixes crashes with the new optimizer enabled noted in #26925

@Keno Keno requested a review from vchuravy May 4, 2018 23:56
@ararslan ararslan added the GC Garbage collector label May 5, 2018
Copy link
Member

@vchuravy vchuravy left a comment

Choose a reason for hiding this comment

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

Thanks Keno! This does indeed fix the issue I noticed.

The only other thing I am seeing on LLVM 6 with the new optimizer enabled is:

Test Failed at /scratch/vchuravy/julia/usr/share/julia/stdlib/v0.7/SparseArrays/test/higherorderfns.jl:269
  Expression: #= /scratch/vchuravy/julia/usr/share/julia/stdlib/v0.7/SparseArrays/test/higherorderfns.jl:269 =# @allocated
(broadcast!(f, Q, X, Y, Z)) == 0
   Evaluated: 16 == 0

which I suppose is meant to be fixed by 26990?

@Keno
Copy link
Member Author

Keno commented May 5, 2018

No, that's #26981.

@Keno
Copy link
Member Author

Keno commented May 5, 2018

CI failures look related. Will take a look.

Keno added 5 commits May 5, 2018 16:15
We stopped using these a while ago when we switched to using LiveSet
diffs for computing where to root values.
The fact that this was absent was masking a bug (fixed in a previous commit).
Now, we don't currently allow any safepoints or allocations before the call
to the ptls getter, but in the future we may want to do things like callee-rooted
arguments, which would have to be rooted across the ptls getter if it were a
safepoint. With the bug fixed, we can proactively add this to the list of
known non-safepoints.
This fixes gc lowering for phis of the form:
```
%phi = phi {%jl_value_t addrspace(10)*, i8} [%aunion, %a], [%bunion, %b]
```
This kind of struct (used by the tagged union representation) has special support
in GC lowering. After optimization, these structs often survive quite far as a
struct (i.e. the destructuring of them is often elided). We simply need to extend
this support to the cases that handle phi and select nodes.

Fixes crashes with the new optimizer enabled noted in #26925
@Keno Keno force-pushed the kf/gcunionphi branch from 8ae878d to b72a570 Compare May 5, 2018 20:21
@vchuravy
Copy link
Member

vchuravy commented May 5, 2018

OSX CI failure seems unrelated:

Error in testset FileWatching:
Test Failed at /private/tmp/julia/share/julia/stdlib/v0.7/FileWatching/test/runtests.jl:401
  Expression: pop!(changes) == (F_PATH => FileWatching.FileEvent(FileWatching.UV_RENAME))
   Evaluated: "afile.txt3" => FileWatching.FileEvent(true, false, false) == "afile.txt" => FileWatching.FileEvent(true, false, false)
Error in testset FileWatching:
Test Failed at /private/tmp/julia/share/julia/stdlib/v0.7/FileWatching/test/runtests.jl:405
  Expression: pop!(changes) == (F_PATH * "~" => FileWatching.FileEvent(FileWatching.UV_RENAME))
   Evaluated: "afile.txt2" => FileWatching.FileEvent(true, false, false) == "afile.txt~" => FileWatching.FileEvent(true, false, false)
Error in testset FileWatching:
Test Failed at /private/tmp/julia/share/julia/stdlib/v0.7/FileWatching/test/runtests.jl:417
  Expression: pop!(changes) == ("$(F_PATH)$(i)" => FileWatching.FileEvent(FileWatching.UV_RENAME))
   Evaluated: "afile.txt" => FileWatching.FileEvent(false, true, false) == "afile.txt3" => FileWatching.FileEvent(true, false, false)
Error in testset FileWatching:
Test Failed at /private/tmp/julia/share/julia/stdlib/v0.7/FileWatching/test/runtests.jl:417
  Expression: pop!(changes) == ("$(F_PATH)$(i)" => FileWatching.FileEvent(FileWatching.UV_RENAME))
   Evaluated: "afile.txt" => FileWatching.FileEvent(true, false, false) == "afile.txt2" => FileWatching.FileEvent(true, false, false)
Error in testset FileWatching:
Test Failed at /private/tmp/julia/share/julia/stdlib/v0.7/FileWatching/test/runtests.jl:417
  Expression: pop!(changes) == ("$(F_PATH)$(i)" => FileWatching.FileEvent(FileWatching.UV_RENAME))
   Evaluated: "afile.txt" => FileWatching.FileEvent(true, false, false) == "afile.txt1" => FileWatching.FileEvent(true, false, false)
ERROR: LoadError: Test run finished with errors
in expression starting at /private/tmp/julia/share/julia/test/runtests.jl:59

@Keno Keno merged commit 10749b9 into master May 6, 2018
@vchuravy vchuravy deleted the kf/gcunionphi branch May 6, 2018 00:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GC Garbage collector
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants