Dead store elimination (draft) #257
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Update `DeadStoreEliminator` prototyping with additional comments and proper function prototype return values. Fix a small warning in `Lifter.h` by adding `llvm::Value` and `llvm::BasicBlock`.
Provide namespaced `remill::StateSlots` function which visits fields of the module's state struct and returns `StateSlot` objects. Remove `comment` field as the `State` object (an `llvm::StructType`) does not track names in a useful way.
Update `DeadStoreEliminator.cpp` to properly produce a vector of `StateSlot`s without type errors (i.e. it compiles). Code still needed for other container types besides structs.
Add state analyzer code for LLVM's `ArrayType` and `VectorType`. Add debugging use of `llvm::Type::dump()`. Consider refactoring as a `StateVisitor` class.
Add code for ForwardAliasVisitor subclass of `llvm::InstVisitor`, to be used for performing alias analysis of lifted functions.
Update ForwardAliasVisitor to use booleans for return types to track when we should add the instruction to the next_wl. Add code to simplify StateSlots for vecs of ints.
Get some of those sweet sweet enums in there!
Allow add and sub instructions with two pointers in the offset map. Add to implementation of visitPHINode.
Add code to create AAMDNodes from StateSlot elements. Change creation of StateSlot vector to have elements for every byte offset of the state structure for fast indexing.
Move function defs up for AAMDNode ops. Add AliasMap typedef. Finish generateAAMDNodesFromSlots.
pgoodman
requested changes
May 25, 2018
remill/BC/DeadStoreEliminator.cpp
Outdated
| std::vector<llvm::Instruction *> insts; | ||
| for (auto &block : func) { | ||
| for (auto &inst : block) { | ||
| insts.push_back(&inst); |
There was a problem hiding this comment.
We can simplify this to do a fav.AddInstruction, as opposed to building up a vector, only to add it into a set.
remill/BC/DeadStoreEliminator.cpp
Outdated
| } | ||
| } | ||
| fav.addInstructions(insts); | ||
| fav.analyze(); |
There was a problem hiding this comment.
Maybe make fav.analyze return true or false, so that we don't add AAMDNodes in the case that the analysis failed.
remill/BC/DeadStoreEliminator.cpp
Outdated
| : i(i_), offset(offset_), size(size_) { } | ||
|
|
||
| StateVisitor::StateVisitor(llvm::DataLayout *dl_) | ||
| : slots(std::vector<StateSlot>()), idx(0), offset(0), dl(dl_) { } |
There was a problem hiding this comment.
Don't need the std::vector<StateSlot>(). slots is already that type, so you can either do slots() to explicitly default-construct it, or even leave slots absent, in which case the compiler will automatically call the default constructor.
remill/BC/DeadStoreEliminator.cpp
Outdated
| } | ||
|
|
||
| StateSlot::StateSlot(uint64_t i_, uint64_t offset_, uint64_t size_) | ||
| : i(i_), offset(offset_), size(size_) { } |
There was a problem hiding this comment.
Coding style change request for constructors:
Foo::Foo(...)
: first_thing_indented_four_spaces,
next_thing_aligned_with_first,
if_just_initializing_then_empty_parens {}
remill/BC/DeadStoreEliminator.cpp
Outdated
| // "slot" of the State structure has its own SlotRecord. | ||
| std::vector<StateSlot> StateSlots(llvm::Module *module) { | ||
| // get the state | ||
| auto slots = std::vector<StateSlot>(); |
There was a problem hiding this comment.
Better to do: std::vector<StateSlot> slots;.
remill/BC/DeadStoreEliminator.h
Outdated
|
|
||
| class LiveSetBlockVisitor { | ||
| public: | ||
| std::unordered_map<llvm::MDNode *, uint64_t> scope_to_offset; |
There was a problem hiding this comment.
make this into a const reference.
remill/BC/DeadStoreEliminator.h
Outdated
| virtual VisitResult visitBinaryOp_(llvm::BinaryOperator &I, bool plus); | ||
| }; | ||
|
|
||
| typedef std::bitset<4096> LiveSet; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
scripts/build.sh
Outdated
| @@ -118,7 +119,8 @@ function GetOSVersion | |||
|
|
|||
| *) | |||
| printf "[x] ${distribution_name} is not yet a supported distribution.\n" | |||
| return 1 | |||
| # FIXME: remove workaround | |||
There was a problem hiding this comment.
Maybe just add in a case for your operating system that makes OS_VERSION into ubuntu1604 and print out a warning? Then this will be an officially supported workaround.
There was a problem hiding this comment.
I'm running Antergos Linux; weirdly enough, cat /etc/issue returns "Antergos Linux \r (\l)", which does weird things to any kind of string manipulation: "${distribution_name}" becomes "(\l)gos Linux". Maybe I can see about some other kind of check we can add.
There was a problem hiding this comment.
Perhaps /etc/os-release/ would be a better file to check?
remill/BC/DeadStoreEliminator.cpp
Outdated
| for (auto inst = B->rbegin(); inst != B->rend(); ++inst) { | ||
| if (inst != B->rbegin() && llvm::isa<llvm::TerminatorInst>(&*inst)) { | ||
| // mark as all live | ||
| block_map[B].first = LiveSet(4095); |
There was a problem hiding this comment.
I am pretty sure that this doesn't do what you think it does. Also, maps/unordered maps default initialize their things. So .first is already a "valid" bitset, with all bits cleared. If you want to set all bits to 1, then what you want is block_map[B].first.set();.
remill/BC/DeadStoreEliminator.cpp
Outdated
| block_map[B].second |= block_map[succ].first; | ||
| } | ||
| for (auto inst = B->rbegin(); inst != B->rend(); ++inst) { | ||
| if (inst != B->rbegin() && llvm::isa<llvm::TerminatorInst>(&*inst)) { |
There was a problem hiding this comment.
A BranchInst is a TerminatorInst.
pgoodman
reviewed
May 29, 2018
remill/BC/DeadStoreEliminator.cpp
Outdated
| //progress = false; | ||
| for (auto block : curr_wl) { | ||
| auto newlive = VisitBlock(block, currlive); | ||
| if (*newlive == *currlive) { |
There was a problem hiding this comment.
This is premature. What you are saying here is: "if any block didn't make progress, then the whole system has converged", yet that is too simplistic. It's more that, if the new and current live sets don't match, then we want to add the predecessors of a block to the worklist.
remill/BC/DeadStoreEliminator.cpp
Outdated
| RemoveDeadStores(to_remove); | ||
| } | ||
|
|
||
| LiveSet *LiveSetBlockVisitor::VisitBlock(llvm::BasicBlock *B, LiveSet *init) { |
There was a problem hiding this comment.
This function can just return a bool about whether or not progress was made.
remill/BC/DeadStoreEliminator.cpp
Outdated
| } | ||
|
|
||
| LiveSet *LiveSetBlockVisitor::VisitBlock(llvm::BasicBlock *B, LiveSet *init) { | ||
| // Set the exit value based on the initial liveset unioned with the successors |
There was a problem hiding this comment.
I suggest you start with something like this:
LiveSet live;
for (auto inst_it = B->rbegin(); inst_it != B->rend(); ++inst_it) {
auto inst = &*inst_it;
if (llvm::isa<llvm::ReturnInst>(inst) || llvm::isa<llvm::UnreachableInst>(inst)) {
// mark all live
} else if (auto br_inst = llvm::dyn_cast<llvm::BrInst>(inst)) {
// update live set from successors
} else if (auto call_inst = llvm::dyn_cast<llvm::CallInst>(inst)) {
// mark all live if the called function type is the same as a lifted function type
} else if (auto invoke_inst = llvm::dyn_cast<llvm::InvokeInst>(inst)) {
// treated like a call
} else if (llvm::isa<llvm::LoadInst>(inst)) {
...
} else if (llvm::isa<llvm::StoreInst>(inst)) {
...
}
}The loop updates live. At the end of the function, you have something like:
auto &old_live_on_entry = block_map[B];
if (old_live_on_entry != live) {
old_live_on_entry = live;
return true;
} else {
return false;
}
Plus clean up the FBV visitor a teensy bit more.
…ling of different types.
…into pag_dead_store
Bringing the family back together.
This commit is to pave the way for future improvements to where this information is calculated (in FAV instead of LSBV).
Move call and invoke arg-based livesets to FAV to allow for module-level LSBV code.
…remill into alberdingk-thijm_dead_store
…ery function per DOT file. Minor tweaks to interprocedural analysis.
…dead but that didn't work out.
…equential type for LLVM 3.8 compatibility, also check for pointer type.
…et analysis terminates but there is still stuff in the work list.
pgoodman
approved these changes
Jun 28, 2018
pgoodman
pushed a commit
that referenced
this pull request
Aug 25, 2018
* CMake: Make the bitcode generator Visual Studio friendly * CMake: Always recompile the runtimes when the semantics change * CMake: Style changes * CMake: Fix missing variable when re-configuring * CMake: Windows/Visual Studio fixes * Windows: Add missing FPU macros * Fix alignment issues with Visual Studio * CMake: Fix the install target for Windows * CMake: Add support for find_package(remill) * Build script: Use verbose makefile * CMake: Add missing ADDRESS_SIZE_BITS in BC compiler * CMake: X86 Tests project refactor * Windows: Port of BC/Utils and OS/FileSystem * CMake: Update settings.cmake * CMake: Generate the semantics path in remillConfig.cmake * CMake: Only use C++14 when compiling on Windows * Rename FPUFlags to Float and collect all FPU flags there * Travis: Add macOS, test all LLVM versions, add clang static analyzer * Travis: Disable LLVM50 tests, include verbose output for failed tests * CMake: Update the find_package handler, add support for fcd headers * Dead store elimination (draft) (#257) * Add initial prototying of DeadStoreEliminator * Improve upon DeadStoreEliminator prototype Update `DeadStoreEliminator` prototyping with additional comments and proper function prototype return values. Fix a small warning in `Lifter.h` by adding `llvm::Value` and `llvm::BasicBlock`. * Add initial code for DeadStoreEliminator Provide namespaced `remill::StateSlots` function which visits fields of the module's state struct and returns `StateSlot` objects. Remove `comment` field as the `State` object (an `llvm::StructType`) does not track names in a useful way. * Update state analyzer and fix type errors Update `DeadStoreEliminator.cpp` to properly produce a vector of `StateSlot`s without type errors (i.e. it compiles). Code still needed for other container types besides structs. * Add remill::VisitSequential for arrays and vectors Add state analyzer code for LLVM's `ArrayType` and `VectorType`. Add debugging use of `llvm::Type::dump()`. Consider refactoring as a `StateVisitor` class. * Begin refactor of Visit funcs to StateVisitor * Fix segfault in StateVisitor::visit, code style * Prototype ForwardAliasVisitor for alias analysis Add code for ForwardAliasVisitor subclass of `llvm::InstVisitor`, to be used for performing alias analysis of lifted functions. * Add non-working visit functions for alias analysis * Fix compilation of alias analysis visit funcs * Change ForwardAliasVisitor to RetTy=bool Update ForwardAliasVisitor to use booleans for return types to track when we should add the instruction to the next_wl. Add code to simplify StateSlots for vecs of ints. * Add progress tracking to AliasAnalysis * Correct progress tracker, use BasicBlockFunction() * Change ForwardAliasAnalysis<RetTy = AliasResult> Get some of those sweet sweet enums in there! * Add FAV state pointer field, PHINode impl * Allow non-const add and sub in FAV Allow add and sub instructions with two pointers in the offset map. Add to implementation of visitPHINode. * Update PHINode impl in FAV * Clean up use of StateSlots to create AAMDNodes Add code to create AAMDNodes from StateSlot elements. Change creation of StateSlot vector to have elements for every byte offset of the state structure for fast indexing. * Complete addition of AAMDNodes for load and store Move function defs up for AAMDNode ops. Add AliasMap typedef. Finish generateAAMDNodesFromSlots. * Add GenerateLiveSet func * Initialize live set, add AAMDNodes to stores * Change LiveSet creation to a block visitor class * Add to_remove set to LSBV * Update build script to use os-release for OS detection * Update DSE code to conform to pag's review * Refactor LiveSetBlockVisitor to one LiveSet per block * Update VisitBlock to better check instruction type * Fix VisitBlock CallInst and InvokeInst cases * Stack allocate AAMDInfo in AnalyzeAliases * Add remove pass option for VisitBlock * Add DOT digraph generation * Fix bugs in dot digraph * Fix various bugs in AAMDNodes and LSBV Move AAMDNodes functions later to match their use. Clean up code conventions. Fix small bugs in various spots in the code. * Fix various function prototypes, overflow checks * Merge AliasMap and OffsetMap * Change add/sub insts to be safer Add OpType enum class to replace use of `plus` bool. Add more straightforward bounds checking on AddInst or SubInst values. * Refactor GetUnsignedOffset style * Fix illegal instruction error * Add offset checking for GEP, provide log messages Write a log message for the cases where `GetUnsignedOffset` returns false (indicating an overflow or underflow). * Fix APInt initialization in VisitGEP * Re-add dead store elimination and alias map * Move LSBV into alias analysis, expand callinst Expand definition of cases where a CallInst should be considered to touch the state struct or otherwise revive a slot. * Improve DOT creation, fix errors for callinsts Fix small errors in LiveSetBlockVisitor::CallAccessesState. Clean up DOT digraph generation further. * Clean up code per @pag's comments * Clean up code, add selectinst, fix compile errors Deal with a few small corner cases with FAV Load and Store instructions, add SelectInst visitor. Inline MarkLiveArgs to avoid the complications of C++ generics. * Clean up logging * More changes to please Peter Clean up select and PHI node cases to improve circular dependency handling. * Correct errors in visitSelect * Add load forwarding code * Add code to run FBV * Fix map usage error in FBV * Add call, invoke cases for FBV * Here are the changes whoops * Minor API change * Minor tweaks to DOT digraph printing, as well as attempts to handle the case where the forward analysis pass is incomplete. Still don't have it guarantee completion on everything, but seems 'good enough' for now. * Change log level * Fix compile errors due to messy merge * Pag dead store (#262) * Fix to script calling wrong function. * Makes sure that value names are preserved (#249) * Adds LLVM_VERSION() to accomodate llvm::LLVMContext::setDiscardValueNames() in <3.9 (#250) * Makes sure that value names are preserved * Adds LLVM_VERSION() macros to accomodate * Update README.md * Update README.md * set state and memory as noalias (#254) * Implements some ring 0 instructions in terms of hyper calls and new I/O port intrinsics. (#252) * Random tests. * More decode error info * more playing around * more system instructions. instrinsics for accessing I/O ports. Split our writes to individual control regs for better identification via hyper calls. * CR8 read/write support (#255) * Check argument index of function (#256) * Fix NoAlias Attributes for older LLVM versions * Fix for LLVM 4.0 and 3.9 * Fix typo * Follow remill coding style * Here are the changes whoops * Minor API change * Minor tweaks to DOT digraph printing, as well as attempts to handle the case where the forward analysis pass is incomplete. Still don't have it guarantee completion on everything, but seems 'good enough' for now. * Change log level * Fix compile errors due to messy merge * Improve call/invoke case of FBV * Improve call/invoke case of FBV * Removes a level of indirection in the __remill_basic_block function * Create dedicated stats tracker Plus clean up the FBV visitor a teensy bit more. * Move call/invoke LiveSet gen to static func This commit is to pave the way for future improvements to where this information is calculated (in FAV instead of LSBV). * Add code to FAV for calls and invokes Move call and invoke arg-based livesets to FAV to allow for module-level LSBV code. * Begin change of LSBV to module-level * Minor bug fixes * Remove some dead code * Begin adding more code for call/invoke LSBV * Add entry block checks for LSBV call/invoke * Info about register names, as well as printing them in the digraphs. * Make DOT printing only happen per function, as opposed to printing every function per DOT file. Minor tweaks to interprocedural analysis. * Bug fixes and DOT printing improvements. * Bug fixes and DOT printing improvements. * Also print out DOT digraphs of functions after removing stuff * Tried to make it treat everything before a call to __remill_error as dead but that didn't work out. * Use datalayout and type sizes to handle the number of elements in a sequential type for LLVM 3.8 compatibility, also check for pointer type. * More stats, hopefully fixes a bitcast issue. * Minor bug fixes, and more DOT printing to help diagnose when the offset analysis terminates but there is still stuff in the work list. * Some possible bug fixes * Minor bug fix * Travis build fixes for earlier compatibility * Fix for LLVM less than 3.8 compatibility. * LLVM 3.5 compatibility * Missing condition in forwarding code that does casting. * LLVM 5.0 compatibility * Add LLVM 6.0 to build.sh. Change default install location to /usr/local * Add back in vmill. Add some extra flags to the building runtimes. * Maybe works * Disable dse across indirect call (#267) * Make DSE sensitive to indirect function calls * FE_DENORM issue * Added some compat code that implements the futimens syscall on mac os 10.12 for travis support. * Disable dse across indirect call (#268) * Make DSE sensitive to indirect function calls * FE_DENORM issue * Added some compat code that implements the futimens syscall on mac os 10.12 for travis support. * Playing with CACHE and PARENT_SCOPE * Minor stack address size check * Fix to DSE I think. * Minor DSE tweak * Update Run.cpp * Update travis.sh Adds in 32-bit libraries for Linux builds.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Draft PR for dead-store elimination code.