<div dir="ltr">Mehdi,<div><br></div><div>The relevant code is:</div><div><div><font face="monospace, monospace">if (int Res = cmpNumbers(L->getValueID(), R->getValueID()))</font></div><div><font face="monospace, monospace"> return Res;</font></div><div><font face="monospace, monospace">if (const auto *SeqL = dyn_cast<ConstantDataSequential>(L)) {<br></font></div><div><font face="monospace, monospace"> const auto *SeqR = dyn_cast<ConstantDataSequential>(R);</font></div></div><div><font face="monospace, monospace"> ...</font></div><div><font face="monospace, monospace">}</font></div><div>This is technically safe because L->getValueID() and R->getValueID() are the same (by the first if statement).</div><div><br></div><div>The second should be a cast<> though, it was a copy-paste error. Good catch!</div><div><br></div><div>-Jason K</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Aug 24, 2015 at 9:21 AM, JF Bastien <span dir="ltr"><<a href="mailto:jfb@google.com" target="_blank">jfb@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Adding Jason, who authored the patch.</div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Aug 22, 2015 at 5:32 PM, Mehdi Amini <span dir="ltr"><<a href="mailto:mehdi.amini@apple.com" target="_blank">mehdi.amini@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
<span><br>
<br>
> On Aug 21, 2015, at 4:27 PM, JF Bastien via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
><br>
> Author: jfb<br>
> Date: Fri Aug 21 18:27:24 2015<br>
> New Revision: 245762<br>
><br>
</span>> URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D245762-26view-3Drev&d=BQIGaQ&c=eEvniauFctOgLOKGJOplqw&r=v-ruWq0KCv2O3thJZiK6naxuXK8mQHZUmGq5FBtAmZ4&m=8AXWbmpresvhRsDs49PoX5gvb_41wieiXuHyjTJ5YMo&s=FoM7SY1rz3BRnUS5DPo6KV91Zjpw7DxGgcFb8AWousg&e=" rel="noreferrer" target="_blank">https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D245762-26view-3Drev&d=BQIGaQ&c=eEvniauFctOgLOKGJOplqw&r=v-ruWq0KCv2O3thJZiK6naxuXK8mQHZUmGq5FBtAmZ4&m=8AXWbmpresvhRsDs49PoX5gvb_41wieiXuHyjTJ5YMo&s=FoM7SY1rz3BRnUS5DPo6KV91Zjpw7DxGgcFb8AWousg&e=</a><br>
<div><div>> Log:<br>
> Improve the determinism of MergeFunctions<br>
><br>
> Summary:<br>
><br>
> Merge functions previously relied on unsigned comparisons of pointer values to<br>
> order functions. This caused observable non-determinism in the compiler for<br>
> large bitcode programs. Basically, opt -mergefuncs program.bc | md5sum produces<br>
> different hashes when run repeatedly on the same machine. Differing output was<br>
> observed on three large bitcodes, but it was less frequent on the smallest file.<br>
> It is possible that this only manifests on the large inputs, hence remaining<br>
> undetected until now.<br>
><br>
> This patch fixes this by removing (almost, see below) all places where<br>
> comparisons between pointers are used to order functions. Most of these changes<br>
> are local, but the comparison of global values requires assigning an identifier<br>
> to each local in the order it is visited. This is very similar to the way the<br>
> comparison function identifies Value*'s defined within a function. Because the<br>
> order of visiting the functions and their subparts is deterministic, the<br>
> identifiers assigned to the globals will be as well, and the order of functions<br>
> will be deterministic.<br>
><br>
> With these changes, there is no more observed non-determinism. There is also<br>
> only minor slowdowns (negligible to 4%) compared to the baseline, which is<br>
> likely a result of the fact that global comparisons involve hash lookups and not<br>
> just pointer comparisons.<br>
><br>
> The one caveat so far is that programs containing BlockAddress constants can<br>
> still be non-deterministic. It is not clear what the right solution is here. In<br>
> particular, even if the global numbers are used to order by function, we still<br>
> need a way to order the BasicBlock*'s. Unfortunately, we cannot just bail out<br>
> and fail to order the functions or consider them equal, because we require a<br>
> total order over functions. Note that programs with BlockAddress constants are<br>
> relatively rare, so the impact of leaving this in is minor as long as this pass<br>
> is opt-in.<br>
><br>
> Author: jrkoenig<br>
><br>
> Reviewers: nlewycky, jfb, dschuff<br>
><br>
> Subscribers: jevinskie, llvm-commits, chapuni<br>
><br>
</div></div>> Differential revision: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D12168&d=BQIGaQ&c=eEvniauFctOgLOKGJOplqw&r=v-ruWq0KCv2O3thJZiK6naxuXK8mQHZUmGq5FBtAmZ4&m=8AXWbmpresvhRsDs49PoX5gvb_41wieiXuHyjTJ5YMo&s=p-0b5PAhXedHTFOoxIbiG0nruh9yDWiUogCym25HyvQ&e=" rel="noreferrer" target="_blank">https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D12168&d=BQIGaQ&c=eEvniauFctOgLOKGJOplqw&r=v-ruWq0KCv2O3thJZiK6naxuXK8mQHZUmGq5FBtAmZ4&m=8AXWbmpresvhRsDs49PoX5gvb_41wieiXuHyjTJ5YMo&s=p-0b5PAhXedHTFOoxIbiG0nruh9yDWiUogCym25HyvQ&e=</a><br>
<span>><br>
> Added:<br>
> llvm/trunk/test/Transforms/MergeFunc/constant-entire-value.ll<br>
> Modified:<br>
> llvm/trunk/lib/Transforms/IPO/MergeFunctions.cpp<br>
><br>
> Modified: llvm/trunk/lib/Transforms/IPO/MergeFunctions.cpp<br>
</span>> URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_lib_Transforms_IPO_MergeFunctions.cpp-3Frev-3D245762-26r1-3D245761-26r2-3D245762-26view-3Ddiff&d=BQIGaQ&c=eEvniauFctOgLOKGJOplqw&r=v-ruWq0KCv2O3thJZiK6naxuXK8mQHZUmGq5FBtAmZ4&m=8AXWbmpresvhRsDs49PoX5gvb_41wieiXuHyjTJ5YMo&s=mdhMReHM3LfubUScGkXKDBloDc16X-lxEn4f_KZhc6Y&e=" rel="noreferrer" target="_blank">https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_lib_Transforms_IPO_MergeFunctions.cpp-3Frev-3D245762-26r1-3D245761-26r2-3D245762-26view-3Ddiff&d=BQIGaQ&c=eEvniauFctOgLOKGJOplqw&r=v-ruWq0KCv2O3thJZiK6naxuXK8mQHZUmGq5FBtAmZ4&m=8AXWbmpresvhRsDs49PoX5gvb_41wieiXuHyjTJ5YMo&s=mdhMReHM3LfubUScGkXKDBloDc16X-lxEn4f_KZhc6Y&e=</a><br>
<div><div>> ==============================================================================<br>
> --- llvm/trunk/lib/Transforms/IPO/MergeFunctions.cpp (original)<br>
> +++ llvm/trunk/lib/Transforms/IPO/MergeFunctions.cpp Fri Aug 21 18:27:24 2015<br>
> @@ -106,6 +106,7 @@<br>
> #include "llvm/IR/Module.h"<br>
> #include "llvm/IR/Operator.h"<br>
> #include "llvm/IR/ValueHandle.h"<br>
> +#include "llvm/IR/ValueMap.h"<br>
> #include "llvm/Pass.h"<br>
> #include "llvm/Support/CommandLine.h"<br>
> #include "llvm/Support/Debug.h"<br>
> @@ -130,14 +131,50 @@ static cl::opt<unsigned> NumFunctionsFor<br>
><br>
> namespace {<br>
><br>
> +/// GlobalNumberState assigns an integer to each global value in the program,<br>
> +/// which is used by the comparison routine to order references to globals. This<br>
> +/// state must be preserved throughout the pass, because Functions and other<br>
> +/// globals need to maintain their relative order. Globals are assigned a number<br>
> +/// when they are first visited. This order is deterministic, and so the<br>
> +/// assigned numbers are as well. When two functions are merged, neither number<br>
> +/// is updated. If the symbols are weak, this would be incorrect. If they are<br>
> +/// strong, then one will be replaced at all references to the other, and so<br>
> +/// direct callsites will now see one or the other symbol, and no update is<br>
> +/// necessary. Note that if we were guaranteed unique names, we could just<br>
> +/// compare those, but this would not work for stripped bitcodes or for those<br>
> +/// few symbols without a name.<br>
> +class GlobalNumberState {<br>
> + struct Config : ValueMapConfig<GlobalValue*> {<br>
> + enum { FollowRAUW = false };<br>
> + };<br>
> + // Each GlobalValue is mapped to an identifier. The Config ensures when RAUW<br>
> + // occurs, the mapping does not change. Tracking changes is unnecessary, and<br>
> + // also problematic for weak symbols (which may be overwritten).<br>
> + typedef ValueMap<GlobalValue *, uint64_t, Config> ValueNumberMap;<br>
> + ValueNumberMap GlobalNumbers;<br>
> + // The next unused serial number to assign to a global.<br>
> + uint64_t NextNumber;<br>
> + public:<br>
> + GlobalNumberState() : GlobalNumbers(), NextNumber(0) {}<br>
> + uint64_t getNumber(GlobalValue* Global) {<br>
> + ValueNumberMap::iterator MapIter;<br>
> + bool Inserted;<br>
> + std::tie(MapIter, Inserted) = GlobalNumbers.insert({Global, NextNumber});<br>
> + if (Inserted)<br>
> + NextNumber++;<br>
> + return MapIter->second;<br>
> + }<br>
> +};<br>
> +<br>
> /// FunctionComparator - Compares two functions to determine whether or not<br>
> /// they will generate machine code with the same behaviour. DataLayout is<br>
> /// used if available. The comparator always fails conservatively (erring on the<br>
> /// side of claiming that two functions are different).<br>
> class FunctionComparator {<br>
> public:<br>
> - FunctionComparator(const Function *F1, const Function *F2)<br>
> - : FnL(F1), FnR(F2) {}<br>
> + FunctionComparator(const Function *F1, const Function *F2,<br>
> + GlobalNumberState* GN)<br>
> + : FnL(F1), FnR(F2), GlobalNumbers(GN) {}<br>
><br>
> /// Test whether the two functions have equivalent behaviour.<br>
> int compare();<br>
> @@ -148,7 +185,7 @@ public:<br>
><br>
> private:<br>
> /// Test whether two basic blocks have equivalent behaviour.<br>
> - int compare(const BasicBlock *BBL, const BasicBlock *BBR);<br>
> + int cmpBasicBlocks(const BasicBlock *BBL, const BasicBlock *BBR);<br>
><br>
> /// Constants comparison.<br>
> /// Its analog to lexicographical comparison between hypothetical numbers<br>
> @@ -254,6 +291,10 @@ private:<br>
> /// If these properties are equal - compare their contents.<br>
> int cmpConstants(const Constant *L, const Constant *R);<br>
><br>
> + /// Compares two global values by number. Uses the GlobalNumbersState to<br>
> + /// identify the same gobals across function calls.<br>
> + int cmpGlobalValues(GlobalValue *L, GlobalValue *R);<br>
> +<br>
> /// Assign or look up previously assigned numbers for the two values, and<br>
> /// return whether the numbers are equal. Numbers are assigned in the order<br>
> /// visited.<br>
> @@ -333,8 +374,9 @@ private:<br>
> ///<br>
> /// 1. If types are of different kind (different type IDs).<br>
> /// Return result of type IDs comparison, treating them as numbers.<br>
> - /// 2. If types are vectors or integers, compare Type* values as numbers.<br>
> - /// 3. Types has same ID, so check whether they belongs to the next group:<br>
> + /// 2. If types are integers, check that they have the same width. If they<br>
> + /// are vectors, check that they have the same count and subtype.<br>
> + /// 3. Types have the same ID, so check whether they are one of:<br>
> /// * Void<br>
> /// * Float<br>
> /// * Double<br>
> @@ -343,8 +385,7 @@ private:<br>
> /// * PPC_FP128<br>
> /// * Label<br>
> /// * Metadata<br>
> - /// If so - return 0, yes - we can treat these types as equal only because<br>
> - /// their IDs are same.<br>
> + /// We can treat these types as equal whenever their IDs are same.<br>
> /// 4. If Left and Right are pointers, return result of address space<br>
> /// comparison (numbers comparison). We can treat pointer types of same<br>
> /// address space as equal.<br>
> @@ -359,7 +400,8 @@ private:<br>
><br>
> int cmpAPInts(const APInt &L, const APInt &R) const;<br>
> int cmpAPFloats(const APFloat &L, const APFloat &R) const;<br>
> - int cmpStrings(StringRef L, StringRef R) const;<br>
> + int cmpInlineAsm(const InlineAsm *L, const InlineAsm *R) const;<br>
> + int cmpMem(StringRef L, StringRef R) const;<br>
> int cmpAttrs(const AttributeSet L, const AttributeSet R) const;<br>
><br>
> // The two functions undergoing comparison.<br>
> @@ -399,33 +441,28 @@ private:<br>
> /// could be operands from further BBs we didn't scan yet.<br>
> /// So it's impossible to use dominance properties in general.<br>
> DenseMap<const Value*, int> sn_mapL, sn_mapR;<br>
> +<br>
> + // The global state we will use<br>
> + GlobalNumberState* GlobalNumbers;<br>
> };<br>
><br>
> class FunctionNode {<br>
> mutable AssertingVH<Function> F;<br>
> FunctionComparator::FunctionHash Hash;<br>
> -<br>
> public:<br>
> // Note the hash is recalculated potentially multiple times, but it is cheap.<br>
> - FunctionNode(Function *F) : F(F), Hash(FunctionComparator::functionHash(*F)){}<br>
> + FunctionNode(Function *F)<br>
> + : F(F), Hash(FunctionComparator::functionHash(*F)) {}<br>
> Function *getFunc() const { return F; }<br>
> + FunctionComparator::FunctionHash getHash() const { return Hash; }<br>
><br>
> /// Replace the reference to the function F by the function G, assuming their<br>
> /// implementations are equal.<br>
> void replaceBy(Function *G) const {<br>
> - assert(!(*this < FunctionNode(G)) && !(FunctionNode(G) < *this) &&<br>
> - "The two functions must be equal");<br>
> -<br>
> F = G;<br>
> }<br>
><br>
> void release() { F = 0; }<br>
> - bool operator<(const FunctionNode &RHS) const {<br>
> - // Order first by hashes, then full function comparison.<br>
> - if (Hash != RHS.Hash)<br>
> - return Hash < RHS.Hash;<br>
> - return (FunctionComparator(F, RHS.getFunc()).compare()) == -1;<br>
> - }<br>
> };<br>
> }<br>
><br>
> @@ -444,13 +481,17 @@ int FunctionComparator::cmpAPInts(const<br>
> }<br>
><br>
> int FunctionComparator::cmpAPFloats(const APFloat &L, const APFloat &R) const {<br>
> - if (int Res = cmpNumbers((uint64_t)&L.getSemantics(),<br>
> - (uint64_t)&R.getSemantics()))<br>
> + // TODO: This correctly handles all existing fltSemantics, because they all<br>
> + // have different precisions. This isn't very robust, however, if new types<br>
> + // with different exponent ranges are introduced.<br>
> + const fltSemantics &SL = L.getSemantics(), &SR = R.getSemantics();<br>
> + if (int Res = cmpNumbers(APFloat::semanticsPrecision(SL),<br>
> + APFloat::semanticsPrecision(SR)))<br>
> return Res;<br>
> return cmpAPInts(L.bitcastToAPInt(), R.bitcastToAPInt());<br>
> }<br>
><br>
> -int FunctionComparator::cmpStrings(StringRef L, StringRef R) const {<br>
> +int FunctionComparator::cmpMem(StringRef L, StringRef R) const {<br>
> // Prevent heavy comparison, compare sizes first.<br>
> if (int Res = cmpNumbers(L.size(), R.size()))<br>
> return Res;<br>
> @@ -556,9 +597,25 @@ int FunctionComparator::cmpConstants(con<br>
> if (!L->isNullValue() && R->isNullValue())<br>
> return -1;<br>
><br>
> + auto GlobalValueL = const_cast<GlobalValue*>(dyn_cast<GlobalValue>(L));<br>
> + auto GlobalValueR = const_cast<GlobalValue*>(dyn_cast<GlobalValue>(R));<br>
> + if (GlobalValueL && GlobalValueR) {<br>
> + return cmpGlobalValues(GlobalValueL, GlobalValueR);<br>
> + }<br>
> +<br>
> if (int Res = cmpNumbers(L->getValueID(), R->getValueID()))<br>
> return Res;<br>
><br>
> + if (const auto *SeqL = dyn_cast<ConstantDataSequential>(L)) {<br>
> + const auto *SeqR = dyn_cast<ConstantDataSequential>(R);<br>
> + // This handles ConstantDataArray and ConstantDataVector. Note that we<br>
> + // compare the two raw data arrays, which might differ depending on the host<br>
> + // endianness. This isn't a problem though, because the endiness of a module<br>
> + // will affect the order of the constants, but this order is the same<br>
> + // for a given input module and host platform.<br>
> + return cmpMem(SeqL->getRawDataValues(), SeqR->getRawDataValues());<br>
<br>
<br>
</div></div>You are dereferencing SeqR after a dyn_cast<>. There is something fishy here.<br>
Can you have a look?<br>
<br>
Thanks,<br>
<br>
Mehdi<br>
<div><div><br>
<br>
<br>
> + }<br>
> +<br>
> switch (L->getValueID()) {<br>
> case Value::UndefValueVal: return TypesRes;<br>
> case Value::ConstantIntVal: {<br>
> @@ -627,12 +684,21 @@ int FunctionComparator::cmpConstants(con<br>
> }<br>
> return 0;<br>
> }<br>
> - case Value::FunctionVal:<br>
> - case Value::GlobalVariableVal:<br>
> - case Value::GlobalAliasVal:<br>
> - default: // Unknown constant, cast L and R pointers to numbers and compare.<br>
> + case Value::BlockAddressVal: {<br>
> + // FIXME: This still uses a pointer comparison. It isn't clear how to remove<br>
> + // this. This only affects programs which take BlockAddresses and store them<br>
> + // as constants, which is limited to interepreters, etc.<br>
> return cmpNumbers((uint64_t)L, (uint64_t)R);<br>
> }<br>
> + default: // Unknown constant, abort.<br>
> + DEBUG(dbgs() << "Looking at valueID " << L->getValueID() << "\n");<br>
> + llvm_unreachable("Constant ValueID not recognized.");<br>
> + return -1;<br>
> + }<br>
> +}<br>
> +<br>
> +int FunctionComparator::cmpGlobalValues(GlobalValue *L, GlobalValue* R) {<br>
> + return cmpNumbers(GlobalNumbers->getNumber(L), GlobalNumbers->getNumber(R));<br>
> }<br>
><br>
> /// cmpType - compares two types,<br>
> @@ -660,10 +726,15 @@ int FunctionComparator::cmpTypes(Type *T<br>
> llvm_unreachable("Unknown type!");<br>
> // Fall through in Release mode.<br>
> case Type::IntegerTyID:<br>
> - case Type::VectorTyID:<br>
> - // TyL == TyR would have returned true earlier.<br>
> - return cmpNumbers((uint64_t)TyL, (uint64_t)TyR);<br>
> -<br>
> + return cmpNumbers(cast<IntegerType>(TyL)->getBitWidth(),<br>
> + cast<IntegerType>(TyR)->getBitWidth());<br>
> + case Type::VectorTyID: {<br>
> + VectorType *VTyL = cast<VectorType>(TyL), *VTyR = cast<VectorType>(TyR);<br>
> + if (int Res = cmpNumbers(VTyL->getNumElements(), VTyR->getNumElements()))<br>
> + return Res;<br>
> + return cmpTypes(VTyL->getElementType(), VTyR->getElementType());<br>
> + }<br>
> + // TyL == TyR would have returned true earlier, because types are uniqued.<br>
> case Type::VoidTyID:<br>
> case Type::FloatTyID:<br>
> case Type::DoubleTyID:<br>
> @@ -895,9 +966,8 @@ int FunctionComparator::cmpGEPs(const GE<br>
> if (GEPL->accumulateConstantOffset(DL, OffsetL) &&<br>
> GEPR->accumulateConstantOffset(DL, OffsetR))<br>
> return cmpAPInts(OffsetL, OffsetR);<br>
> -<br>
> - if (int Res = cmpNumbers((uint64_t)GEPL->getPointerOperand()->getType(),<br>
> - (uint64_t)GEPR->getPointerOperand()->getType()))<br>
> + if (int Res = cmpTypes(GEPL->getPointerOperand()->getType(),<br>
> + GEPR->getPointerOperand()->getType()))<br>
> return Res;<br>
><br>
> if (int Res = cmpNumbers(GEPL->getNumOperands(), GEPR->getNumOperands()))<br>
> @@ -911,6 +981,28 @@ int FunctionComparator::cmpGEPs(const GE<br>
> return 0;<br>
> }<br>
><br>
> +int FunctionComparator::cmpInlineAsm(const InlineAsm *L,<br>
> + const InlineAsm *R) const {<br>
> + // InlineAsm's are uniqued. If they are the same pointer, obviously they are<br>
> + // the same, otherwise compare the fields.<br>
> + if (L == R)<br>
> + return 0;<br>
> + if (int Res = cmpTypes(L->getFunctionType(), R->getFunctionType()))<br>
> + return Res;<br>
> + if (int Res = cmpMem(L->getAsmString(), R->getAsmString()))<br>
> + return Res;<br>
> + if (int Res = cmpMem(L->getConstraintString(), R->getConstraintString()))<br>
> + return Res;<br>
> + if (int Res = cmpNumbers(L->hasSideEffects(), R->hasSideEffects()))<br>
> + return Res;<br>
> + if (int Res = cmpNumbers(L->isAlignStack(), R->isAlignStack()))<br>
> + return Res;<br>
> + if (int Res = cmpNumbers(L->getDialect(), R->getDialect()))<br>
> + return Res;<br>
> + llvm_unreachable("InlineAsm blocks were not uniqued.");<br>
> + return 0;<br>
> +}<br>
> +<br>
> /// Compare two values used by the two functions under pair-wise comparison. If<br>
> /// this is the first time the values are seen, they're added to the mapping so<br>
> /// that we will detect mismatches on next use.<br>
> @@ -945,7 +1037,7 @@ int FunctionComparator::cmpValues(const<br>
> const InlineAsm *InlineAsmR = dyn_cast<InlineAsm>(R);<br>
><br>
> if (InlineAsmL && InlineAsmR)<br>
> - return cmpNumbers((uint64_t)L, (uint64_t)R);<br>
> + return cmpInlineAsm(InlineAsmL, InlineAsmR);<br>
> if (InlineAsmL)<br>
> return 1;<br>
> if (InlineAsmR)<br>
> @@ -957,7 +1049,8 @@ int FunctionComparator::cmpValues(const<br>
> return cmpNumbers(LeftSN.first->second, RightSN.first->second);<br>
> }<br>
> // Test whether two basic blocks have equivalent behaviour.<br>
> -int FunctionComparator::compare(const BasicBlock *BBL, const BasicBlock *BBR) {<br>
> +int FunctionComparator::cmpBasicBlocks(const BasicBlock *BBL,<br>
> + const BasicBlock *BBR) {<br>
> BasicBlock::const_iterator InstL = BBL->begin(), InstLE = BBL->end();<br>
> BasicBlock::const_iterator InstR = BBR->begin(), InstRE = BBR->end();<br>
><br>
> @@ -1020,7 +1113,7 @@ int FunctionComparator::compare() {<br>
> return Res;<br>
><br>
> if (FnL->hasGC()) {<br>
> - if (int Res = cmpNumbers((uint64_t)FnL->getGC(), (uint64_t)FnR->getGC()))<br>
> + if (int Res = cmpMem(FnL->getGC(), FnR->getGC()))<br>
> return Res;<br>
> }<br>
><br>
> @@ -1028,7 +1121,7 @@ int FunctionComparator::compare() {<br>
> return Res;<br>
><br>
> if (FnL->hasSection()) {<br>
> - if (int Res = cmpStrings(FnL->getSection(), FnR->getSection()))<br>
> + if (int Res = cmpMem(FnL->getSection(), FnR->getSection()))<br>
> return Res;<br>
> }<br>
><br>
> @@ -1074,7 +1167,7 @@ int FunctionComparator::compare() {<br>
> if (int Res = cmpValues(BBL, BBR))<br>
> return Res;<br>
><br>
> - if (int Res = compare(BBL, BBR))<br>
> + if (int Res = cmpBasicBlocks(BBL, BBR))<br>
> return Res;<br>
><br>
> const TerminatorInst *TermL = BBL->getTerminator();<br>
> @@ -1129,7 +1222,7 @@ FunctionComparator::FunctionHash Functio<br>
> SmallVector<const BasicBlock *, 8> BBs;<br>
> SmallSet<const BasicBlock *, 16> VisitedBBs;<br>
><br>
> - // Walk the blocks in the same order as FunctionComparator::compare(),<br>
> + // Walk the blocks in the same order as FunctionComparator::cmpBasicBlocks(),<br>
> // accumulating the hash of the function "structure." (BB and opcode sequence)<br>
> BBs.push_back(&F.getEntryBlock());<br>
> VisitedBBs.insert(BBs[0]);<br>
> @@ -1163,14 +1256,31 @@ class MergeFunctions : public ModulePass<br>
> public:<br>
> static char ID;<br>
> MergeFunctions()<br>
> - : ModulePass(ID), HasGlobalAliases(false) {<br>
> + : ModulePass(ID), FnTree(FunctionNodeCmp(&GlobalNumbers)),<br>
> + HasGlobalAliases(false) {<br>
> initializeMergeFunctionsPass(*PassRegistry::getPassRegistry());<br>
> }<br>
><br>
> bool runOnModule(Module &M) override;<br>
><br>
> private:<br>
> - typedef std::set<FunctionNode> FnTreeType;<br>
> + // The function comparison operator is provided here so that FunctionNodes do<br>
> + // not need to become larger with another pointer.<br>
> + class FunctionNodeCmp {<br>
> + GlobalNumberState* GlobalNumbers;<br>
> + public:<br>
> + FunctionNodeCmp(GlobalNumberState* GN) : GlobalNumbers(GN) {}<br>
> + bool operator()(const FunctionNode &LHS, const FunctionNode &RHS) const {<br>
> + // Order first by hashes, then full function comparison.<br>
> + if (LHS.getHash() != RHS.getHash())<br>
> + return LHS.getHash() < RHS.getHash();<br>
> + FunctionComparator FCmp(LHS.getFunc(), RHS.getFunc(), GlobalNumbers);<br>
> + return FCmp.compare() == -1;<br>
> + }<br>
> + };<br>
> + typedef std::set<FunctionNode, FunctionNodeCmp> FnTreeType;<br>
> +<br>
> + GlobalNumberState GlobalNumbers;<br>
><br>
> /// A work queue of functions that may have been modified and should be<br>
> /// analyzed again.<br>
> @@ -1245,8 +1355,8 @@ bool MergeFunctions::doSanityCheck(std::<br>
> for (std::vector<WeakVH>::iterator J = I; J != E && j < Max; ++J, ++j) {<br>
> Function *F1 = cast<Function>(*I);<br>
> Function *F2 = cast<Function>(*J);<br>
> - int Res1 = FunctionComparator(F1, F2).compare();<br>
> - int Res2 = FunctionComparator(F2, F1).compare();<br>
> + int Res1 = FunctionComparator(F1, F2, &GlobalNumbers).compare();<br>
> + int Res2 = FunctionComparator(F2, F1, &GlobalNumbers).compare();<br>
><br>
> // If F1 <= F2, then F2 >= F1, otherwise report failure.<br>
> if (Res1 != -Res2) {<br>
> @@ -1267,8 +1377,8 @@ bool MergeFunctions::doSanityCheck(std::<br>
> continue;<br>
><br>
> Function *F3 = cast<Function>(*K);<br>
> - int Res3 = FunctionComparator(F1, F3).compare();<br>
> - int Res4 = FunctionComparator(F2, F3).compare();<br>
> + int Res3 = FunctionComparator(F1, F3, &GlobalNumbers).compare();<br>
> + int Res4 = FunctionComparator(F2, F3, &GlobalNumbers).compare();<br>
><br>
> bool Transitive = true;<br>
><br>
> @@ -1556,6 +1666,8 @@ void MergeFunctions::replaceFunctionInTr<br>
> (!F->mayBeOverridden() && !G->mayBeOverridden())) &&<br>
> "Only change functions if both are strong or both are weak");<br>
> (void)F;<br>
> + assert(FunctionComparator(F, G, &GlobalNumbers).compare() == 0 &&<br>
> + "The two functions must be equal");<br>
><br>
> IterToF->replaceBy(G);<br>
> }<br>
><br>
> Added: llvm/trunk/test/Transforms/MergeFunc/constant-entire-value.ll<br>
</div></div>> URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_test_Transforms_MergeFunc_constant-2Dentire-2Dvalue.ll-3Frev-3D245762-26view-3Dauto&d=BQIGaQ&c=eEvniauFctOgLOKGJOplqw&r=v-ruWq0KCv2O3thJZiK6naxuXK8mQHZUmGq5FBtAmZ4&m=8AXWbmpresvhRsDs49PoX5gvb_41wieiXuHyjTJ5YMo&s=XV-m4M5ffPZ4yM6fTcQlJGawM37EGxaZ-ixc9lXpz7w&e=" rel="noreferrer" target="_blank">https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_test_Transforms_MergeFunc_constant-2Dentire-2Dvalue.ll-3Frev-3D245762-26view-3Dauto&d=BQIGaQ&c=eEvniauFctOgLOKGJOplqw&r=v-ruWq0KCv2O3thJZiK6naxuXK8mQHZUmGq5FBtAmZ4&m=8AXWbmpresvhRsDs49PoX5gvb_41wieiXuHyjTJ5YMo&s=XV-m4M5ffPZ4yM6fTcQlJGawM37EGxaZ-ixc9lXpz7w&e=</a><br>
<div><div>> ==============================================================================<br>
> --- llvm/trunk/test/Transforms/MergeFunc/constant-entire-value.ll (added)<br>
> +++ llvm/trunk/test/Transforms/MergeFunc/constant-entire-value.ll Fri Aug 21 18:27:24 2015<br>
> @@ -0,0 +1,42 @@<br>
> +; RUN: opt -S -mergefunc < %s | FileCheck %s<br>
> +<br>
> +; RUN: opt -S -mergefunc < %s | FileCheck -check-prefix=NOPLUS %s<br>
> +<br>
> +; This makes sure that zeros in constants don't cause problems with string based<br>
> +; memory comparisons<br>
> +define internal i32 @sum(i32 %x, i32 %y) {<br>
> +; CHECK-LABEL: @sum<br>
> + %sum = add i32 %x, %y<br>
> + %1 = extractvalue [3 x i32] [ i32 3, i32 0, i32 2 ], 2<br>
> + %sum2 = add i32 %sum, %1<br>
> + %sum3 = add i32 %sum2, %y<br>
> + ret i32 %sum3<br>
> +}<br>
> +<br>
> +define internal i32 @add(i32 %x, i32 %y) {<br>
> +; CHECK-LABEL: @add<br>
> + %sum = add i32 %x, %y<br>
> + %1 = extractvalue [3 x i32] [ i32 3, i32 0, i32 1 ], 2<br>
> + %sum2 = add i32 %sum, %1<br>
> + %sum3 = add i32 %sum2, %y<br>
> + ret i32 %sum3<br>
> +}<br>
> +<br>
> +define internal i32 @plus(i32 %x, i32 %y) {<br>
> +; NOPLUS-NOT: @plus<br>
> + %sum = add i32 %x, %y<br>
> + %1 = extractvalue [3 x i32] [ i32 3, i32 0, i32 5 ], 2<br>
> + %sum2 = add i32 %sum, %1<br>
> + %sum3 = add i32 %sum2, %y<br>
> + ret i32 %sum3<br>
> +}<br>
> +<br>
> +define internal i32 @next(i32 %x, i32 %y) {<br>
> +; CHECK-LABEL: @next<br>
> + %sum = add i32 %x, %y<br>
> + %1 = extractvalue [3 x i32] [ i32 3, i32 0, i32 5 ], 2<br>
> + %sum2 = add i32 %sum, %1<br>
> + %sum3 = add i32 %sum2, %y<br>
> + ret i32 %sum3<br>
> +}<br>
> +<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
</div></div>> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_llvm-2Dcommits&d=BQIGaQ&c=eEvniauFctOgLOKGJOplqw&r=v-ruWq0KCv2O3thJZiK6naxuXK8mQHZUmGq5FBtAmZ4&m=8AXWbmpresvhRsDs49PoX5gvb_41wieiXuHyjTJ5YMo&s=UpG70G8xAGjnGmVMInrkg28NaSY3FySnGQbQHEpKGaY&e=" rel="noreferrer" target="_blank">https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_llvm-2Dcommits&d=BQIGaQ&c=eEvniauFctOgLOKGJOplqw&r=v-ruWq0KCv2O3thJZiK6naxuXK8mQHZUmGq5FBtAmZ4&m=8AXWbmpresvhRsDs49PoX5gvb_41wieiXuHyjTJ5YMo&s=UpG70G8xAGjnGmVMInrkg28NaSY3FySnGQbQHEpKGaY&e=</a><br>
<br>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>