[PATCH] GlobalOpt enhancement optimization (hoisting of initialization code into constant global initializers).

[Michael Ilseman]

I'm not familiar with global-opt, so hopefully someone who is can comment on the design.

Here's some low-level feedback:

+  std::vector<ReturnInst*> GVreturnInstUses;
+  std::vector<StoreInst*> GVstoreInstUses;
+  std::set<Function*> initFunction; // weed out dupes

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.

+  // More initializer type checks. Better to do them early.
+  Type *GVinitType = GV->getInitializer()->getType();
+  ArrayType *AT = dyn_cast<ArrayType>(GVinitType);
+  StructType *ST = dyn_cast<StructType>(GVinitType);
+  IntegerType *IT = dyn_cast<IntegerType>(GVinitType);
+  if (!ST && !AT && !IT) return false;
+
+  unsigned numTypes = IT ? 1 : 0; // 1 for scalar (IT != NULL)
+  if (ST)
+    numTypes = GVinitType->getNumContainedTypes();
+  else if (AT)
+    numTypes = AT->getNumElements();
+
+  // Quit early for empty structs and arrays.
+  if (numTypes == 0) return false;

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.

+  pred_iterator PI = pred_begin(defBB), PE = pred_end(defBB);
+  BasicBlock *predBB = *PI;
+  unsigned predCount = 0;
+  for (; PI != PE; ++PI, predCount++);

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.

+  BranchInst *brinst = predCount == 1 ?
+    dyn_cast<BranchInst>(predBB->getTerminator()) : NULL;
+
+  // Only if the predecessor of the defBB ends
+  // with a conditional branch can it actually be
+  // an initializer. It must initialize only the
+  // first time that it is called to initialize.
+  if (!brinst || !brinst->isConditional()) return false;

I don't think we like to use NULL.

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.

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.


[1] http://llvm.org/docs/ProgrammersManual.html#picking-the-right-data-structure-for-a-task
[2] http://llvm.org/docs/CodingStandards.html
[3] http://llvm.org/docs/doxygen/html/PatternMatch_8h.html

On Aug 26, 2013, at 1:40 AM, Puyan Lotfi <plotfi at apple.com> wrote:

> All:
> 
> 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). 
> 
> The idea is to transform the following code:
> 
> A = internal global i32 0, align 4
> isInit = internal global i1 false
> 
> define i32* foobar() {
>   %.b = load i1* isInit
>   %1 = zext i1 %.b to i8
>   %2 = trunc i8 %1 to i1
>   br i1 %2, BB4, label BB3
> BB3:
>   store i32 113, i32* A, align 4
>   store i1 true, i1* isInit
>   br label %4
> BB4:
>   ret i32* A
> }
> 
> Into:
> 
> A = internal global i32 113, align 4
> 
> define i32* @_Z8initTestv() {
>   ret i32* A
> }
> 
> Could someone on the list review my changes, provide feedback, and if possible submit my changes?
> 
> 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). 
> 
> Thanks
> 
> -Puyan
> 
> 
> <GlobalOptInitHoist.patch>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130826/577f09d2/attachment.html>