<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Hey JF, Jason<div class=""><br class=""></div><div class="">There’s an ASan/UBSan failure with a mergefunc test on the bots. The blame list range is r245138-r245153.</div><div class=""><br class=""></div><div class="">Mind taking a look to see if this commit might be the cause. Its the only MergeFunc related one I could see in the range.</div><div class=""><br class=""></div><div class=""><a href="http://lab.llvm.org:8080/green/job/clang-stage2-cmake-RgSan_check/94/consoleFull#-121115394049ba4694-19c4-4d7e-bec5-911270d8a58c" class="">http://lab.llvm.org:8080/green/job/clang-stage2-cmake-RgSan_check/94/consoleFull#-121115394049ba4694-19c4-4d7e-bec5-911270d8a58c</a></div><div class=""><br class=""></div><div class="">Cheers,</div><div class="">Pete</div><div class=""><div><blockquote type="cite" class=""><div class="">On Aug 14, 2015, at 7:06 PM, JF Bastien via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><p dir="ltr" class="">I'll defer the answer to Jason, who authored the patch. </p>
<div class="gmail_quote">On Aug 14, 2015 6:39 PM, "David Blaikie" <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:<br type="attribution" class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class=""><br class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Fri, Aug 14, 2015 at 6:18 PM, JF Bastien via llvm-commits <span dir="ltr" class=""><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: jfb<br class="">
Date: Fri Aug 14 20:18:18 2015<br class="">
New Revision: 245140<br class="">
<br class="">
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=245140&view=rev" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project?rev=245140&view=rev</a><br class="">
Log:<br class="">
Accelerate MergeFunctions with hashing<br class="">
<br class="">
This patch makes the Merge Functions pass faster by calculating and comparing<br class="">
a hash value which captures the essential structure of a function before<br class="">
performing a full function comparison.<br class="">
<br class="">
The hash is calculated by hashing the function signature, then walking the basic<br class="">
blocks of the function in the same order as the main comparison function. The<br class="">
opcode of each instruction is hashed in sequence, which means that different<br class="">
functions according to the existing total order cannot have the same hash, as<br class="">
the comparison requires the opcodes of the two functions to be the same order.<br class="">
<br class="">
The hash function is a static member of the FunctionComparator class because it<br class="">
is tightly coupled to the exact comparison function used. For example, functions<br class="">
which are equivalent modulo a single variant callsite might be merged by a more<br class="">
aggressive MergeFunctions, and the hash function would need to be insensitive to<br class="">
these differences in order to exploit this.<br class="">
<br class="">
The hashing function uses a utility class which accumulates the values into an<br class="">
internal state using a standard bit-mixing function. Note that this is a different interface<br class="">
than a regular hashing routine, because the values to be hashed are scattered<br class="">
amongst the properties of a llvm::Function, not linear in memory. This scheme is<br class="">
fast because only one word of state needs to be kept, and the mixing function is<br class="">
a few instructions.<br class="">
<br class="">
The main runOnModule function first computes the hash of each function, and only<br class="">
further processes functions which do not have a unique function hash. The hash<br class="">
is also used to order the sorted function set. If the hashes differ, their<br class="">
values are used to order the functions, otherwise the full comparison is done.<br class="">
<br class="">
Both of these are helpful in speeding up MergeFunctions. Together they result in<br class="">
speedups of 9% for mysqld (a mostly C application with little redundancy), 46%<br class="">
for libxul in Firefox, and 117% for Chromium. (These are all LTO builds.) In all<br class="">
three cases, the new speed of MergeFunctions is about half that of the module<br class="">
verifier, making it relatively inexpensive even for large LTO builds with<br class="">
hundreds of thousands of functions. The same functions are merged, so this<br class="">
change is free performance.<br class="">
<br class="">
Author: jrkoenig<br class="">
<br class="">
Reviewers: nlewycky, dschuff, jfb<br class="">
<br class="">
Subscribers: llvm-commits, aemerson<br class="">
<br class="">
Differential revision: <a href="http://reviews.llvm.org/D11923" rel="noreferrer" target="_blank" class="">http://reviews.llvm.org/D11923</a><br class="">
<br class="">
Modified:<br class="">
llvm/trunk/lib/Transforms/IPO/MergeFunctions.cpp<br class="">
llvm/trunk/test/Transforms/MergeFunc/call-and-invoke-with-ranges.ll<br class="">
<br class="">
Modified: llvm/trunk/lib/Transforms/IPO/MergeFunctions.cpp<br class="">
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/MergeFunctions.cpp?rev=245140&r1=245139&r2=245140&view=diff" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/MergeFunctions.cpp?rev=245140&r1=245139&r2=245140&view=diff</a><br class="">
==============================================================================<br class="">
--- llvm/trunk/lib/Transforms/IPO/MergeFunctions.cpp (original)<br class="">
+++ llvm/trunk/lib/Transforms/IPO/MergeFunctions.cpp Fri Aug 14 20:18:18 2015<br class="">
@@ -27,6 +27,14 @@<br class="">
// -- We define Function* container class with custom "operator<" (FunctionPtr).<br class="">
// -- "FunctionPtr" instances are stored in std::set collection, so every<br class="">
// std::set::insert operation will give you result in log(N) time.<br class="">
+//<br class="">
+// As an optimization, a hash of the function structure is calculated first, and<br class="">
+// two functions are only compared if they have the same hash. This hash is<br class="">
+// cheap to compute, and has the property that if function F == G according to<br class="">
+// the comparison function, then hash(F) == hash(G). This consistency property<br class="">
+// is critical to ensuring all possible merging opportunities are exploited.<br class="">
+// Collisions in the hash affect the speed of the pass but not the correctness<br class="">
+// or determinism of the resulting transformation.<br class="">
//<br class="">
// When a match is found the functions are folded. If both functions are<br class="">
// overridable, we move the functionality into a new internal function and<br class="">
@@ -87,6 +95,7 @@<br class="">
#include "llvm/ADT/STLExtras.h"<br class="">
#include "llvm/ADT/SmallSet.h"<br class="">
#include "llvm/ADT/Statistic.h"<br class="">
+#include "llvm/ADT/Hashing.h"<br class="">
#include "llvm/IR/CallSite.h"<br class="">
#include "llvm/IR/Constants.h"<br class="">
#include "llvm/IR/DataLayout.h"<br class="">
@@ -132,6 +141,10 @@ public:<br class="">
<br class="">
/// Test whether the two functions have equivalent behaviour.<br class="">
int compare();<br class="">
+ /// Hash a function. Equivalent functions will have the same hash, and unequal<br class="">
+ /// functions will have different hashes with high probability.<br class="">
+ typedef uint64_t FunctionHash;<br class="">
+ static FunctionHash functionHash(Function &);<br class="">
<br class="">
private:<br class="">
/// Test whether two basic blocks have equivalent behaviour.<br class="">
@@ -390,9 +403,11 @@ private:<br class="">
<br class="">
class FunctionNode {<br class="">
mutable AssertingVH<Function> F;<br class="">
+ FunctionComparator::FunctionHash Hash;<br class="">
<br class="">
public:<br class="">
- FunctionNode(Function *F) : F(F) {}<br class="">
+ // Note the hash is recalculated potentially multiple times, but it is cheap.<br class="">
+ FunctionNode(Function *F) : F(F), Hash(FunctionComparator::functionHash(*F)){}<br class="">
Function *getFunc() const { return F; }<br class="">
<br class="">
/// Replace the reference to the function F by the function G, assuming their<br class="">
@@ -406,6 +421,9 @@ public:<br class="">
<br class="">
void release() { F = 0; }<br class="">
bool operator<(const FunctionNode &RHS) const {<br class="">
+ // Order first by hashes, then full function comparison.<br class="">
+ if (Hash != RHS.Hash)<br class="">
+ return Hash < RHS.Hash;<br class="">
return (FunctionComparator(F, RHS.getFunc()).compare()) == -1;<br class="">
}<br class="">
};<br class="">
@@ -1074,6 +1092,66 @@ int FunctionComparator::compare() {<br class="">
return 0;<br class="">
}<br class="">
<br class="">
+// Accumulate the hash of a sequence of 64-bit integers. This is similar to a<br class="">
+// hash of a sequence of 64bit ints, but the entire input does not need to be<br class="">
+// available at once. This interface is necessary for functionHash because it<br class="">
+// needs to accumulate the hash as the structure of the function is traversed<br class="">
+// without saving these values to an intermediate buffer. This form of hashing<br class="">
+// is not often needed, as usually the object to hash is just read from a<br class="">
+// buffer.<br class="">
+class HashAccumulator64 {<br class=""></blockquote><div class=""><br class="">Any chance of using LLVM's Hashing.h for hash aggregation? (it's already got accumulator support, if I'm not mistaken?)<br class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ uint64_t Hash;<br class="">
+public:<br class="">
+ // Initialize to random constant, so the state isn't zero.<br class="">
+ HashAccumulator64() { Hash = 0x6acaa36bef8325c5ULL; }<br class="">
+ void add(uint64_t V) {<br class="">
+ Hash = llvm::hashing::detail::hash_16_bytes(Hash, V);<br class="">
+ }<br class="">
+ // No finishing is required, because the entire hash value is used.<br class="">
+ uint64_t getHash() { return Hash; }<br class="">
+};<br class="">
+<br class="">
+// A function hash is calculated by considering only the number of arguments and<br class="">
+// whether a function is varargs, the order of basic blocks (given by the<br class="">
+// successors of each basic block in depth first order), and the order of<br class="">
+// opcodes of each instruction within each of these basic blocks. This mirrors<br class="">
+// the strategy compare() uses to compare functions by walking the BBs in depth<br class="">
+// first order and comparing each instruction in sequence. Because this hash<br class="">
+// does not look at the operands, it is insensitive to things such as the<br class="">
+// target of calls and the constants used in the function, which makes it useful<br class="">
+// when possibly merging functions which are the same modulo constants and call<br class="">
+// targets.<br class="">
+FunctionComparator::FunctionHash FunctionComparator::functionHash(Function &F) {<br class="">
+ HashAccumulator64 H;<br class="">
+ H.add(F.isVarArg());<br class="">
+ H.add(F.arg_size());<br class="">
+<br class="">
+ SmallVector<const BasicBlock *, 8> BBs;<br class="">
+ SmallSet<const BasicBlock *, 16> VisitedBBs;<br class="">
+<br class="">
+ // Walk the blocks in the same order as FunctionComparator::compare(),<br class="">
+ // accumulating the hash of the function "structure." (BB and opcode sequence)<br class="">
+ BBs.push_back(&F.getEntryBlock());<br class="">
+ VisitedBBs.insert(BBs[0]);<br class="">
+ while (!BBs.empty()) {<br class="">
+ const BasicBlock *BB = BBs.pop_back_val();<br class="">
+ // This random value acts as a block header, as otherwise the partition of<br class="">
+ // opcodes into BBs wouldn't affect the hash, only the order of the opcodes<br class="">
+ H.add(45798);<br class="">
+ for (auto &Inst : *BB) {<br class="">
+ H.add(Inst.getOpcode());<br class="">
+ }<br class="">
+ const TerminatorInst *Term = BB->getTerminator();<br class="">
+ for (unsigned i = 0, e = Term->getNumSuccessors(); i != e; ++i) {<br class="">
+ if (!VisitedBBs.insert(Term->getSuccessor(i)).second)<br class="">
+ continue;<br class="">
+ BBs.push_back(Term->getSuccessor(i));<br class="">
+ }<br class="">
+ }<br class="">
+ return H.getHash();<br class="">
+}<br class="">
+<br class="">
+<br class="">
namespace {<br class="">
<br class="">
/// MergeFunctions finds functions which will generate identical machine code,<br class="">
@@ -1228,11 +1306,28 @@ bool MergeFunctions::doSanityCheck(std::<br class="">
bool MergeFunctions::runOnModule(Module &M) {<br class="">
bool Changed = false;<br class="">
<br class="">
- for (Module::iterator I = M.begin(), E = M.end(); I != E; ++I) {<br class="">
- if (!I->isDeclaration() && !I->hasAvailableExternallyLinkage())<br class="">
- Deferred.push_back(WeakVH(I));<br class="">
+ // All functions in the module, ordered by hash. Functions with a unique<br class="">
+ // hash value are easily eliminated.<br class="">
+ std::vector<std::pair<FunctionComparator::FunctionHash, Function *>><br class="">
+ HashedFuncs;<br class="">
+ for (Function &Func : M) {<br class="">
+ if (!Func.isDeclaration() && !Func.hasAvailableExternallyLinkage()) {<br class="">
+ HashedFuncs.push_back({FunctionComparator::functionHash(Func), &Func});<br class="">
+ }<br class="">
+ }<br class="">
+<br class="">
+ std::sort(HashedFuncs.begin(), HashedFuncs.end());<br class="">
+<br class="">
+ auto S = HashedFuncs.begin();<br class="">
+ for (auto I = HashedFuncs.begin(), IE = HashedFuncs.end(); I != IE; ++I) {<br class="">
+ // If the hash value matches the previous value or the next one, we must<br class="">
+ // consider merging it. Otherwise it is dropped and never considered again.<br class="">
+ if ((I != S && std::prev(I)->first == I->first) ||<br class="">
+ (std::next(I) != IE && std::next(I)->first == I->first) ) {<br class="">
+ Deferred.push_back(WeakVH(I->second));<br class="">
+ }<br class="">
}<br class="">
-<br class="">
+<br class="">
do {<br class="">
std::vector<WeakVH> Worklist;<br class="">
Deferred.swap(Worklist);<br class="">
<br class="">
Modified: llvm/trunk/test/Transforms/MergeFunc/call-and-invoke-with-ranges.ll<br class="">
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/MergeFunc/call-and-invoke-with-ranges.ll?rev=245140&r1=245139&r2=245140&view=diff" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/MergeFunc/call-and-invoke-with-ranges.ll?rev=245140&r1=245139&r2=245140&view=diff</a><br class="">
==============================================================================<br class="">
--- llvm/trunk/test/Transforms/MergeFunc/call-and-invoke-with-ranges.ll (original)<br class="">
+++ llvm/trunk/test/Transforms/MergeFunc/call-and-invoke-with-ranges.ll Fri Aug 14 20:18:18 2015<br class="">
@@ -63,14 +63,6 @@ lpad:<br class="">
resume { i8*, i32 } zeroinitializer<br class="">
}<br class="">
<br class="">
-define i8 @call_with_same_range() {<br class="">
-; CHECK-LABEL: @call_with_same_range<br class="">
-; CHECK: tail call i8 @call_with_range<br class="">
- bitcast i8 0 to i8<br class="">
- %out = call i8 @dummy(), !range !0<br class="">
- ret i8 %out<br class="">
-}<br class="">
-<br class="">
define i8 @invoke_with_same_range() personality i8* undef {<br class="">
; CHECK-LABEL: @invoke_with_same_range()<br class="">
; CHECK: tail call i8 @invoke_with_range()<br class="">
@@ -84,6 +76,16 @@ lpad:<br class="">
resume { i8*, i32 } zeroinitializer<br class="">
}<br class="">
<br class="">
+define i8 @call_with_same_range() {<br class="">
+; CHECK-LABEL: @call_with_same_range<br class="">
+; CHECK: tail call i8 @call_with_range<br class="">
+ bitcast i8 0 to i8<br class="">
+ %out = call i8 @dummy(), !range !0<br class="">
+ ret i8 %out<br class="">
+}<br class="">
+<br class="">
+<br class="">
+<br class="">
declare i8 @dummy();<br class="">
declare i32 @__gxx_personality_v0(...)<br class="">
<br class="">
<br class="">
<br class="">
_______________________________________________<br class="">
llvm-commits mailing list<br class="">
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a><br class="">
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank" class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br class="">
</blockquote></div><br class=""></div></div>
</blockquote></div>
_______________________________________________<br class="">llvm-commits mailing list<br class=""><a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a><br class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits<br class=""></div></blockquote></div><br class=""></div></body></html>