<div dir="ltr">I reverted it because it was having issues on one of the linux buildbots, and I didn't have access to a linux machine last week. Going to take a look at the issue tomorrow.<div><br></div><div>Sorry about reverting through git, I'll use the script next time.</div><div><br></div><div> - Anthony</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jul 27, 2015 at 11:09 AM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br>
> On 2015-Jul-22, at 15:26, Anthony Pesch <<a href="mailto:inolen@gmail.com">inolen@gmail.com</a>> wrote:<br>
><br>
> Author: inolen<br>
> Date: Wed Jul 22 17:26:54 2015<br>
> New Revision: 242954<br>
><br>
> URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D242954-26view-3Drev&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=9W2LbSGshTdrGc2afBUjr1yNw115FvToWLd6gqrs9gQ&s=b960vePabwBjC3ocfMeUY8nsP1GGRJL4Nh3cWSqHAlk&e=" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=242954&view=rev</a><br>
> Log:<br>
> Revert "Improve merging of stores from static constructors in GlobalOpt"<br>
<br>
</span>Why?<br>
<br>
> This reverts commit 0a9dee959a30b81b9e7df64c9a58ff9898c24024.<br>
<br>
Please use SVN revisions when you revert. We have a script at<br>
utils/git-svn/git-svnrevert that you can use to automatically<br>
substitute the SVN revision in for the git hash.<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
> Modified:<br>
> llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp<br>
><br>
> Modified: llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp<br>
> URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_lib_Transforms_IPO_GlobalOpt.cpp-3Frev-3D242954-26r1-3D242953-26r2-3D242954-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=9W2LbSGshTdrGc2afBUjr1yNw115FvToWLd6gqrs9gQ&s=rMxWnSSsHdfp-Pdcp2XOPwi8SmzM58FBjn5aSxoe9ec&e=" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp?rev=242954&r1=242953&r2=242954&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp (original)<br>
> +++ llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp Wed Jul 22 17:26:54 2015<br>
> @@ -1963,240 +1963,6 @@ bool GlobalOpt::OptimizeGlobalVars(Modul<br>
> return Changed;<br>
> }<br>
><br>
> -namespace {<br>
> -<br>
> -/// Sorts GEP expressions in ascending order by their indexes.<br>
> -struct GEPComparator {<br>
> - bool operator()(GEPOperator *A, GEPOperator *B) const {<br>
> - int NumOpA = A->getNumOperands();<br>
> - int NumOpB = B->getNumOperands();<br>
> -<br>
> - // Globals are always pointers, the first index should be 0.<br>
> - assert(cast<ConstantInt>(A->getOperand(1))->isZero() &&<br>
> - "GEP A steps over object");<br>
> - assert(cast<ConstantInt>(B->getOperand(1))->isZero() &&<br>
> - "GEP B steps over object");<br>
> -<br>
> - for (int i = 2; i < NumOpA && i < NumOpB; i++) {<br>
> - ConstantInt *IndexA = cast<ConstantInt>(A->getOperand(i));<br>
> - ConstantInt *IndexB = cast<ConstantInt>(B->getOperand(i));<br>
> -<br>
> - if (IndexA->getZExtValue() < IndexB->getZExtValue()) {<br>
> - return true;<br>
> - }<br>
> - }<br>
> -<br>
> - return NumOpA < NumOpB;<br>
> - }<br>
> -};<br>
> -<br>
> -typedef std::map<GEPOperator *, Constant *, GEPComparator> StoreMap;<br>
> -<br>
> -/// MutatedGlobal - Holds mutations for a global. If a store overwrites the<br>
> -/// the entire global, Initializer is updated with the new value. If a store<br>
> -/// writes to a GEP of a global, the store is instead added to the Pending<br>
> -/// map to be merged later during MergePendingStores.<br>
> -struct MutatedGlobal {<br>
> - GlobalVariable *GV;<br>
> - Constant *Initializer;<br>
> - StoreMap Pending;<br>
> -};<br>
> -<br>
> -/// MutatedGlobals - This class tracks and commits stores to globals as basic<br>
> -/// blocks are evaluated.<br>
> -class MutatedGlobals {<br>
> - DenseMap<GlobalVariable *, MutatedGlobal> Globals;<br>
> - typedef DenseMap<GlobalVariable *, MutatedGlobal>::const_iterator<br>
> - const_iterator;<br>
> -<br>
> - GlobalVariable *GetGlobalForPointer(Constant *Ptr) {<br>
> - if (GlobalVariable *GV = dyn_cast<GlobalVariable>(Ptr)) {<br>
> - return GV;<br>
> - }<br>
> -<br>
> - if (ConstantExpr *CE = dyn_cast<ConstantExpr>(Ptr)) {<br>
> - if (CE->getOpcode() == Instruction::GetElementPtr) {<br>
> - return cast<GlobalVariable>(CE->getOperand(0));<br>
> - }<br>
> - }<br>
> -<br>
> - return nullptr;<br>
> - }<br>
> -<br>
> - Constant *MergePendingStores(Constant *Init, StoreMap &Pending,<br>
> - uint64_t CurrentIdx, unsigned OpNum);<br>
> -<br>
> -public:<br>
> - const_iterator begin() const { return Globals.begin(); }<br>
> - const_iterator end() const { return Globals.end(); }<br>
> - size_t size() const { return Globals.size(); }<br>
> -<br>
> - void AddStore(Constant *Ptr, Constant *Value);<br>
> - Constant *LookupStore(Constant *Ptr);<br>
> -<br>
> - void Commit(MutatedGlobal &MG);<br>
> -};<br>
> -}<br>
> -<br>
> -/// AddStore - Add store for the global variable referenced by Ptr.<br>
> -/// Currently, it's assumed that the incoming pointer is either the global<br>
> -/// variable itself, or a GEP expression referencing the global.<br>
> -void MutatedGlobals::AddStore(Constant *Ptr, Constant *Value) {<br>
> - GlobalVariable *GV = GetGlobalForPointer(Ptr);<br>
> - assert(GV && "Failed to resolve global for pointer");<br>
> -<br>
> - auto I = Globals.find(GV);<br>
> - if (I == Globals.end()) {<br>
> - auto R = Globals.insert(std::make_pair(GV, MutatedGlobal{GV, nullptr, {}}));<br>
> - assert(R.second && "Global value already in the map?");<br>
> - I = R.first;<br>
> - }<br>
> -<br>
> - MutatedGlobal &MG = I->second;<br>
> -<br>
> - if (Ptr == GV) {<br>
> - MG.Initializer = Value;<br>
> - // Pending stores are no longer valid.<br>
> - MG.Pending.clear();<br>
> - } else if (GEPOperator *GEPOp = dyn_cast<GEPOperator>(Ptr)) {<br>
> - MG.Pending[GEPOp] = Value;<br>
> - } else {<br>
> - llvm_unreachable("Unexpected address type");<br>
> - }<br>
> -}<br>
> -<br>
> -Constant *MutatedGlobals::LookupStore(Constant *Ptr) {<br>
> - GlobalVariable *GV = GetGlobalForPointer(Ptr);<br>
> - if (!GV) {<br>
> - return nullptr;<br>
> - }<br>
> -<br>
> - auto I = Globals.find(GV);<br>
> - if (I == Globals.end()) {<br>
> - return nullptr;<br>
> - }<br>
> -<br>
> - MutatedGlobal &MG = I->second;<br>
> -<br>
> - if (Ptr == MG.GV) {<br>
> - if (MG.Initializer) {<br>
> - // If there are any pending stores, Initializer isn't valid, it would<br>
> - // need them merged in first. This situation currently doesn't occur<br>
> - // due to isSimpleEnoughPointerToCommit / isSimpleEnoughValueToCommit<br>
> - // not letting stores for aggregate types pass through. If this needs<br>
> - // to be supported, calling Commit() at this point should do the trick.<br>
> - assert(MG.Pending.empty() &&<br>
> - "Can't use pending initializer without merging pending stores.");<br>
> - return MG.Initializer;<br>
> - }<br>
> - } else if (GEPOperator *GEPOp = dyn_cast<GEPOperator>(Ptr)) {<br>
> - auto SI = MG.Pending.find(GEPOp);<br>
> - if (SI != MG.Pending.end()) {<br>
> - return SI->second;<br>
> - }<br>
> - }<br>
> -<br>
> - return nullptr;<br>
> -}<br>
> -<br>
> -/// MergePendingStores - Recursively merge stores to a global variable into its<br>
> -/// initializer. Merging any number of stores into the initializer requires<br>
> -/// cloning the entire initializer, so stores are batched up during evaluation<br>
> -/// and processed all at once.<br>
> -Constant *MutatedGlobals::MergePendingStores(Constant *Init, StoreMap &Pending,<br>
> - uint64_t CurrentIdx,<br>
> - unsigned OpNum) {<br>
> - if (Pending.empty()) {<br>
> - // Nothing left to merge.<br>
> - return Init;<br>
> - }<br>
> -<br>
> - // If the GEP expression has been traversed completely, terminate.<br>
> - auto It = Pending.begin();<br>
> - GEPOperator *GEP = It->first;<br>
> -<br>
> - if (OpNum >= GEP->getNumOperands()) {<br>
> - Constant *Val = It->second;<br>
> - assert(Val->getType() == Init->getType() && "Type mismatch!");<br>
> -<br>
> - // Move on to the next expression.<br>
> - Pending.erase(It++);<br>
> -<br>
> - return Val;<br>
> - }<br>
> -<br>
> - // Clone the existing initializer so it can be merged into.<br>
> - Type *InitTy = Init->getType();<br>
> - ArrayType *ATy = dyn_cast<ArrayType>(InitTy);<br>
> - StructType *STy = dyn_cast<StructType>(InitTy);<br>
> - VectorType *VTy = dyn_cast<VectorType>(InitTy);<br>
> -<br>
> - unsigned NumElts;<br>
> - if (ATy) {<br>
> - NumElts = ATy->getNumElements();<br>
> - } else if (STy) {<br>
> - NumElts = STy->getNumElements();<br>
> - } else if (VTy) {<br>
> - NumElts = VTy->getNumElements();<br>
> - } else {<br>
> - llvm_unreachable("Unexpected initializer type");<br>
> - }<br>
> -<br>
> - SmallVector<Constant *, 32> Elts;<br>
> - for (unsigned i = 0; i < NumElts; ++i) {<br>
> - Elts.push_back(Init->getAggregateElement(i));<br>
> - }<br>
> -<br>
> - // Iterate over the sorted stores, merging all stores for the current GEP<br>
> - // index.<br>
> - while (!Pending.empty()) {<br>
> - It = Pending.begin();<br>
> - GEP = It->first;<br>
> -<br>
> - // If the store doesn't belong to the current index, we're done.<br>
> - ConstantInt *CI = cast<ConstantInt>(GEP->getOperand(OpNum - 1));<br>
> - uint64_t Idx = CI->getZExtValue();<br>
> - if (Idx != CurrentIdx) {<br>
> - break;<br>
> - }<br>
> -<br>
> - // Recurse into the next index.<br>
> - CI = cast<ConstantInt>(GEP->getOperand(OpNum));<br>
> - Idx = CI->getZExtValue();<br>
> - assert(Idx < NumElts && "GEP index out of range!");<br>
> - Elts[Idx] = MergePendingStores(Elts[Idx], Pending, Idx, OpNum + 1);<br>
> - }<br>
> -<br>
> - if (ATy) {<br>
> - return ConstantArray::get(ATy, Elts);<br>
> - } else if (STy) {<br>
> - return ConstantStruct::get(STy, Elts);<br>
> - } else if (VTy) {<br>
> - return ConstantVector::get(Elts);<br>
> - } else {<br>
> - llvm_unreachable("Unexpected initializer type");<br>
> - }<br>
> -<br>
> - return nullptr;<br>
> -};<br>
> -<br>
> -/// Commit - We have decided that stores to the global (which satisfy the<br>
> -/// predicate isSimpleEnoughPointerToCommit) should be committed.<br>
> -void MutatedGlobals::Commit(MutatedGlobal &MG) {<br>
> - Constant *Init = MG.Initializer ? MG.Initializer : MG.GV->getInitializer();<br>
> -<br>
> - // Globals are always pointers, skip first GEP index assuming it's 0.<br>
> - Init = MergePendingStores(Init, MG.Pending, 0, 2);<br>
> -<br>
> - // Reset pending state.<br>
> - MG.Initializer = nullptr;<br>
> - assert(MG.Pending.empty() &&<br>
> - "Expected pending stores to be empty after merging");<br>
> -<br>
> - MG.GV->setInitializer(Init);<br>
> -}<br>
> -<br>
> -<br>
> static inline bool<br>
> isSimpleEnoughValueToCommit(Constant *C,<br>
> SmallPtrSetImpl<Constant *> &SimpleConstants,<br>
> @@ -2329,6 +2095,69 @@ static bool isSimpleEnoughPointerToCommi<br>
> return false;<br>
> }<br>
><br>
> +/// EvaluateStoreInto - Evaluate a piece of a constantexpr store into a global<br>
> +/// initializer. This returns 'Init' modified to reflect 'Val' stored into it.<br>
> +/// At this point, the GEP operands of Addr [0, OpNo) have been stepped into.<br>
> +static Constant *EvaluateStoreInto(Constant *Init, Constant *Val,<br>
> + ConstantExpr *Addr, unsigned OpNo) {<br>
> + // Base case of the recursion.<br>
> + if (OpNo == Addr->getNumOperands()) {<br>
> + assert(Val->getType() == Init->getType() && "Type mismatch!");<br>
> + return Val;<br>
> + }<br>
> +<br>
> + SmallVector<Constant*, 32> Elts;<br>
> + if (StructType *STy = dyn_cast<StructType>(Init->getType())) {<br>
> + // Break up the constant into its elements.<br>
> + for (unsigned i = 0, e = STy->getNumElements(); i != e; ++i)<br>
> + Elts.push_back(Init->getAggregateElement(i));<br>
> +<br>
> + // Replace the element that we are supposed to.<br>
> + ConstantInt *CU = cast<ConstantInt>(Addr->getOperand(OpNo));<br>
> + unsigned Idx = CU->getZExtValue();<br>
> + assert(Idx < STy->getNumElements() && "Struct index out of range!");<br>
> + Elts[Idx] = EvaluateStoreInto(Elts[Idx], Val, Addr, OpNo+1);<br>
> +<br>
> + // Return the modified struct.<br>
> + return ConstantStruct::get(STy, Elts);<br>
> + }<br>
> +<br>
> + ConstantInt *CI = cast<ConstantInt>(Addr->getOperand(OpNo));<br>
> + SequentialType *InitTy = cast<SequentialType>(Init->getType());<br>
> +<br>
> + uint64_t NumElts;<br>
> + if (ArrayType *ATy = dyn_cast<ArrayType>(InitTy))<br>
> + NumElts = ATy->getNumElements();<br>
> + else<br>
> + NumElts = InitTy->getVectorNumElements();<br>
> +<br>
> + // Break up the array into elements.<br>
> + for (uint64_t i = 0, e = NumElts; i != e; ++i)<br>
> + Elts.push_back(Init->getAggregateElement(i));<br>
> +<br>
> + assert(CI->getZExtValue() < NumElts);<br>
> + Elts[CI->getZExtValue()] =<br>
> + EvaluateStoreInto(Elts[CI->getZExtValue()], Val, Addr, OpNo+1);<br>
> +<br>
> + if (Init->getType()->isArrayTy())<br>
> + return ConstantArray::get(cast<ArrayType>(InitTy), Elts);<br>
> + return ConstantVector::get(Elts);<br>
> +}<br>
> +<br>
> +/// CommitValueTo - We have decided that Addr (which satisfies the predicate<br>
> +/// isSimpleEnoughPointerToCommit) should get Val as its value. Make it happen.<br>
> +static void CommitValueTo(Constant *Val, Constant *Addr) {<br>
> + if (GlobalVariable *GV = dyn_cast<GlobalVariable>(Addr)) {<br>
> + assert(GV->hasInitializer());<br>
> + GV->setInitializer(Val);<br>
> + return;<br>
> + }<br>
> +<br>
> + ConstantExpr *CE = cast<ConstantExpr>(Addr);<br>
> + GlobalVariable *GV = cast<GlobalVariable>(CE->getOperand(0));<br>
> + GV->setInitializer(EvaluateStoreInto(GV->getInitializer(), Val, CE, 2));<br>
> +}<br>
> +<br>
> namespace {<br>
><br>
> /// Evaluator - This class evaluates LLVM IR, producing the Constant<br>
> @@ -2373,8 +2202,8 @@ public:<br>
> ValueStack.back()[V] = C;<br>
> }<br>
><br>
> - MutatedGlobals &getMutated() {<br>
> - return Mutated;<br>
> + const DenseMap<Constant*, Constant*> &getMutatedMemory() const {<br>
> + return MutatedMemory;<br>
> }<br>
><br>
> const SmallPtrSetImpl<GlobalVariable*> &getInvariants() const {<br>
> @@ -2394,10 +2223,10 @@ private:<br>
> /// unbounded.<br>
> SmallVector<Function*, 4> CallStack;<br>
><br>
> - /// Mutated - For each store we execute, we update this map. Loads check<br>
> - /// this to get the most up-to-date value. If evaluation is successful,<br>
> + /// MutatedMemory - For each store we execute, we update this map. Loads<br>
> + /// check this to get the most up-to-date value. If evaluation is successful,<br>
> /// this state is committed to the process.<br>
> - MutatedGlobals Mutated;<br>
> + DenseMap<Constant*, Constant*> MutatedMemory;<br>
><br>
> /// AllocaTmps - To 'execute' an alloca, we create a temporary global variable<br>
> /// to represent its body. This vector is needed so we can delete the<br>
> @@ -2424,8 +2253,8 @@ private:<br>
> Constant *Evaluator::ComputeLoadResult(Constant *P) {<br>
> // If this memory location has been recently stored, use the stored value: it<br>
> // is the most up-to-date.<br>
> - Constant *Val = Mutated.LookupStore(P);<br>
> - if (Val) return Val;<br>
> + DenseMap<Constant*, Constant*>::const_iterator I = MutatedMemory.find(P);<br>
> + if (I != MutatedMemory.end()) return I->second;<br>
><br>
> // Access it.<br>
> if (GlobalVariable *GV = dyn_cast<GlobalVariable>(P)) {<br>
> @@ -2529,7 +2358,7 @@ bool Evaluator::EvaluateBlock(BasicBlock<br>
> }<br>
> }<br>
><br>
> - Mutated.AddStore(Ptr, Val);<br>
> + MutatedMemory[Ptr] = Val;<br>
> } else if (BinaryOperator *BO = dyn_cast<BinaryOperator>(CurInst)) {<br>
> InstResult = ConstantExpr::get(BO->getOpcode(),<br>
> getVal(BO->getOperand(0)),<br>
> @@ -2865,13 +2694,12 @@ static bool EvaluateStaticConstructor(Fu<br>
><br>
> // We succeeded at evaluation: commit the result.<br>
> DEBUG(dbgs() << "FULLY EVALUATED GLOBAL CTOR FUNCTION '"<br>
> - << F->getName() << "' to " << Eval.getMutated().size()<br>
> - << " mutated globals.\n");<br>
> -<br>
> - MutatedGlobals &Mutated = Eval.getMutated();<br>
> - for (auto I : Mutated)<br>
> - Mutated.Commit(I.second);<br>
> -<br>
> + << F->getName() << "' to " << Eval.getMutatedMemory().size()<br>
> + << " stores.\n");<br>
> + for (DenseMap<Constant*, Constant*>::const_iterator I =<br>
> + Eval.getMutatedMemory().begin(), E = Eval.getMutatedMemory().end();<br>
> + I != E; ++I)<br>
> + CommitValueTo(I->second, I->first);<br>
> for (GlobalVariable *GV : Eval.getInvariants())<br>
> GV->setConstant(true);<br>
> }<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br>
</div></div></blockquote></div><br></div>