<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>I'm not familiar with global-opt, so hopefully someone who is can comment on the design.</div><div><br></div><div>Here's some low-level feedback:</div><div><br></div><div>+ std::vector<ReturnInst*> GVreturnInstUses;</div><div>+ std::vector<StoreInst*> GVstoreInstUses;</div><div>+ std::set<Function*> initFunction; // weed out dupes</div></div><div><br></div><div>What is the size of these in the common case? If it's a handful, use Small* variants. If it's quite a bit, then reserve for the vectors, and consider a DenseMap instead of the set. Read over [1] for more detail, it's an interesting and useful review of LLVM data structures. This also applies to the vectors scattered throughout the rest of the code.</div><div><br></div><div><div>+ // More initializer type checks. Better to do them early.</div><div>+ Type *GVinitType = GV->getInitializer()->getType();</div><div>+ ArrayType *AT = dyn_cast<ArrayType>(GVinitType);</div><div>+ StructType *ST = dyn_cast<StructType>(GVinitType);</div><div>+ IntegerType *IT = dyn_cast<IntegerType>(GVinitType);</div><div>+ if (!ST && !AT && !IT) return false;</div><div>+</div><div>+ unsigned numTypes = IT ? 1 : 0; // 1 for scalar (IT != NULL)</div><div>+ if (ST)</div><div>+ numTypes = GVinitType->getNumContainedTypes();</div><div>+ else if (AT)</div><div>+ numTypes = AT->getNumElements();</div><div>+</div><div>+ // Quit early for empty structs and arrays.</div><div>+ if (numTypes == 0) return false;</div></div><div><br></div><div>Why not have an if (isa<StructType>(…)) … else if (ArrayType *AT = dyn_cast<ArrayType>(…)) … else if (…) … pattern here? This seems prone to bugs upon modification, and is less clear. AT, ST, and IT also don't appear again until hundreds of lines later in the source.</div><div><br></div><div><div>+ pred_iterator PI = pred_begin(defBB), PE = pred_end(defBB);</div><div>+ BasicBlock *predBB = *PI;</div><div>+ unsigned predCount = 0;</div><div>+ for (; PI != PE; ++PI, predCount++);</div></div><div><br></div><div>Why not have PI/PE be scoped to the loop? I don't see any immediate uses following, so you can put them in the loop header.</div><div><br></div><div><div>+ BranchInst *brinst = predCount == 1 ?</div><div>+ dyn_cast<BranchInst>(predBB->getTerminator()) : NULL;</div><div>+</div><div>+ // Only if the predecessor of the defBB ends</div><div>+ // with a conditional branch can it actually be</div><div>+ // an initializer. It must initialize only the</div><div>+ // first time that it is called to initialize.</div><div>+ if (!brinst || !brinst->isConditional()) return false;</div></div><div><br></div><div>I don't think we like to use NULL.</div><div><br></div><div>Nitpicks: There's also coding convention. I notice you use a comment pattern that's inconsistent with the rest of the code (a full line of /////////// before and after every comment). Keep it consistent. Follow the surrounding code when present, else fall back to the standards guide [2]. There's a lot of lower-case variables, missing spaces after "if", etc. Run a spell checker on your comments.</div><div><br></div><div>Also, I don't know if it's fitting in the style of the rest of GlobalOpt, but consider using pattern matchers [3] to help simplify some of your if-then-else chains. It may not make sense here, but you might find it useful eventually. Try to consider if there's a way to extract some of the utility code into static inline helper functions if it makes sense. As Meador pointed out, please include test cases in patch.</div><div><br></div><div><br></div><div>[1] <a href="http://llvm.org/docs/ProgrammersManual.html#picking-the-right-data-structure-for-a-task">http://llvm.org/docs/ProgrammersManual.html#picking-the-right-data-structure-for-a-task</a></div><div>[2] <a href="http://llvm.org/docs/CodingStandards.html">http://llvm.org/docs/CodingStandards.html</a></div><div>[3] <a href="http://llvm.org/docs/doxygen/html/PatternMatch_8h.html">http://llvm.org/docs/doxygen/html/PatternMatch_8h.html</a></div><br><div><div>On Aug 26, 2013, at 1:40 AM, Puyan Lotfi <<a href="mailto:plotfi@apple.com">plotfi@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=us-ascii"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>All:</div><div><br></div><div>Attached is a patch to GlobalOpt that adds an optimization that takes user written constant initialization code for global variables and hoists the initialization values into the global initializer. This optimization is only done on locally linked globals of integral types (scalars, arrays, and structs) that are constant initialized (and zero initialized prior to transformation). </div><div><br></div><div>The idea is to transform the following code:</div><div><br></div><div><div><font face="Courier New">A = internal global i32 0, align 4</font></div><div><font face="Courier New">isInit = internal global i1 false</font></div><div><font face="Courier New"><br></font></div><div><font face="Courier New">define i32* foobar() {</font></div><div><font face="Courier New"> %.b = load i1* isInit</font></div><div><font face="Courier New"> %1 = zext i1 %.b to i8</font></div><div><font face="Courier New"> %2 = trunc i8 %1 to i1</font></div><div><font face="Courier New"> br i1 %2, BB4, label BB3</font></div><div><font face="Courier New">BB3:</font></div><div><font face="Courier New"> store i32 113, i32* A, align 4</font></div><div><font face="Courier New"> store i1 true, i1* isInit</font></div><div><font face="Courier New"> br label %4</font></div><div><font face="Courier New">BB4:</font></div><div><font face="Courier New"> ret i32* A</font></div><div><font face="Courier New">}</font></div><div><br></div><div>Into:</div><div><font face="Courier New"><br></font></div><div><font face="Courier New">A = internal global i32 113, align 4</font></div><div><font face="Courier New"><br></font></div><div><font face="Courier New">define i32* @_Z8initTestv() {</font></div><div><font face="Courier New"> ret i32* A</font></div><div><font face="Courier New">}</font></div><div><br></div></div><div>Could someone on the list review my changes, provide feedback, and if possible submit my changes?</div><div><br></div><div>I also have some test cases I've written but I am still trying to figure out how to add them to llvm/test/Transforms/GlobalOpt (I don't see a lit.local.cfg in that directory as the docs specify). </div><div><br></div><div>Thanks</div><div><br></div><div>-Puyan</div><div><br></div><div><br></div></div><span><GlobalOptInitHoist.patch></span><meta http-equiv="Content-Type" content="text/html charset=us-ascii"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "></div>
_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits<br></blockquote></div><br></body></html>