-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Delete unneeded GC Roots (fixed #20981) #21158
Conversation
See lengthy comment in llvm-gcroot.cpp for a description of the algorithm.
Cool :) |
I just realized this has a bug for certain loop patterns. Will fix later. |
@@ -7339,6 +7344,15 @@ extern "C" void jl_dump_llvm_value(void *v) | |||
((Value*)v)->dump(); | |||
#endif | |||
} | |||
|
|||
extern "C" void jl_dump_llvm_to_file(char *fname, llvm::Function *F) { | |||
auto M = CloneModule(F->getParent()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be 4 space indents
Fixed the loop case. |
Trailing whitespace in:
|
@@ -124,7 +104,7 @@ ifeq ($(USE_LLVM_SHLIB),1) | |||
# NOTE: we could also --disable-static here (on the condition we link tools | |||
# against libLLVM) but there doesn't seem to be a CMake counterpart option | |||
LLVM_FLAGS += --enable-shared | |||
LLVM_CMAKE += -DLLVM_BUILD_LLVM_DYLIB:BOOL=ON | |||
LLVM_CMAKE += -DLLVM_BUILD_LLVM_DYLIB:BOOL=ON -DLLVM_LINK_LLVM_DYLIB:BOOL=ON |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what version of llvm first had this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely certain but it's no earlier than 3.7.0.
* - "hard kill": We can prove a GC root will no longer be used | ||
* - "soft kill": An opaque safe point was encountered, we don't know | ||
* anything, | ||
* about whether this root is needed anymore. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Odd line splitting / wrapping / indentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comma also not needed after "anything"
config.test_source_root = os.path.dirname(__file__) | ||
config.test_format = lit.formats.ShTest(False) | ||
|
||
path = os.path.pathsep.join((os.path.join(os.path.dirname(__file__),"../../usr/tools"), config.environment['PATH'])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should separate out the string into arguments to os.path.join
Perhaps it would be good to have some of this discussion in the dev docs. |
This works basically fine, but I think a better solution would be a proper iterative data flow approach to so both optimizations and placement in one go. I'll split some of the LLVM test infrastructure I wrote here into a separate PR and then put up a new PR for a better algorithm. |
Leave the branch until the replacement is done ;) |
Right, sorry |
See lengthy comment in llvm-gcroot.cpp for a description of the algorithm.