-
Notifications
You must be signed in to change notification settings - Fork 440
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
LLVM 4 #18
LLVM 4 #18
Changes from all commits
54415ad
73f69af
ea5c7d4
9abbd56
fdab9cd
ef3bb89
b62082f
cab36c2
af491dc
5fbd66b
de31c27
7b7e39d
837d2d2
099e767
e5c9dd3
37111d4
29ae917
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
Release/ | ||
Debug/ | ||
build/ | ||
build-master/ | ||
html/ | ||
Release+Asserts/ | ||
Debug+Asserts/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
Diff between “llvm-4” and “master” | ||
|
||
Likely the most important difference is the requirement that a NodeRef (rather than NodeType) be declared for types that will utilize the GraphTraits as part of the class definition. | ||
|
||
I altered most of the getPassName() instances to return a llvm::StringRef rather than a const char * type. This seems rather trivial and I didn’t see a reason not to use the preferred string type choice for LLVM. | ||
|
||
/include/Util/DataFlow.h | ||
The PostDominatorTree becomes PostDominatorTreeWrapperPass, which is pretty consistent in recent revisions where a pass is accessed via a wrapper. | ||
|
||
I altered the base class /include/Util/GraphUtil.h | ||
|
||
To have a typedef typename Traits::NodeRef NodeRef, per llvm 4. I believe this is necessary, as I looked over the llvm-4. This with the NodeRef alterations in each of the specific graph classes required some alterations in terms of dereferencing, etc. Again, a quick pass and it seemed to work but this is a place where a simple “*” might have a subtle unintended consequence. | ||
|
||
include/WPA/WPAPass.h | ||
|
||
Here I had to remove the explicit inheritance from “AliasAnalysis”, and this likely needs a fix, as AliasAnalysis needs a TargetLibraryInfo to be passed as the argument now and I’m a little unsure as to where to get that from. There is no more default constructor… To get the TargetLibraryInfo it seems the best convention is: | ||
|
||
TLInfo = &getAnalysis<TargetLibraryInfoWrapperPass>().getTLI(); | ||
|
||
But it didn’t seem to “drop in” to the wpa tool perfectly. It seems that inheriting from AAResultsWrapperPass might make sense? Or to have a dependency there? That said, I’m still learning so I wanted to point this out and let others more familiar make that decision. | ||
|
||
I had a number of CMAKE issues, most of which I was able to solve but might have a few redundancies, perhaps that is because I’m on a Mac. | ||
|
||
I believe the rest is pretty straightforward. Looking back the WPAPass is likely an issue to be solved prior to merging into master, the rest |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,6 +51,12 @@ set(SOURCES | |
add_llvm_loadable_module(Svf ${SOURCES}) | ||
add_llvm_Library(LLVMSvf ${SOURCES}) | ||
|
||
# It seems that the Svf library needs to explictly require the CUDD alloc/init/etc so LLVMCudd | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The newly added part causes an error when linking "Svf.so" in my ubuntu machine (completely ok on mac), see below:
If I delete those five lines, it will be all good on ubuntu, but causing a linking error on mac os ;( If possible, could you please take a look at it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, this ought to be straightforward I think. I just moved computers but let me create a VM to test in and put in a architecture check and I think this can be resolved... (I'll try to update tonight). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe I have a fix but have been in VM purgatory today so but basically just used CMAKE_SYSTEM_NAME and a little if-else.. so just will test again, today got away from me but this is the easy part... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright, this built for me on Ubuntu16.04 LT with the following changes: I can reissue the pull request if that's easier but this is trivial so I'm going to focus on the iterator issue below. |
||
link_directories( ${CMAKE_BINARY_DIR}/lib/Cudd ) | ||
|
||
llvm_map_components_to_libnames(llvm_libs bitwriter core ipo irreader instcombine instrumentation target linker analysis scalaropts support ) | ||
target_link_libraries(LLVMSvf ${llvm_libs}) | ||
target_link_libraries(Svf LLVMCudd ${llvm_libs}) | ||
|
||
if(DEFINED IN_SOURCE_BUILD) | ||
add_dependencies(Svf intrinsics_gen) | ||
|
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.
looks like this snuck in!
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.
oh wow, sorry, I can update that if need be.. obviously just a typo... :(