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

[Nick Lewycky]

On 26 August 2013 01:40, 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?
>

You'll need to provide testcases before we can submit it for you.


> 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).
>

+/// variables and if the condtions are right proceeds to move the init

Typo, "condtions" --> "conditions"

+/// The following is an example of code we would like to optimize:
+/// S* init () {
+///   static bool isInit = false; static S s;
+///   if (!isInit) { s.x = 42; s.y = 21; isInit = true; }
+///   return &s;
+/// }

I'm curious, is this a pattern people actually write?

+static bool TryToHoistGlobalConstantInitializers(GlobalVariable *GV) {
+  bool isInitCandidate = true;
+  BasicBlock* defBB = NULL;

"BasicBlock* defBB" --> "BasicBlock *defBB".

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

Please see
http://llvm.org/docs/ProgrammersManual.html#picking-the-right-data-structure-for-a-task.

+  // Only consider globals that are interally linked, and constant
initialized.

Typo, "interally" --> "internally".

+  // also return a referenece.

Typo, "referenece" --> "reference".

+  bool GVChanged;
+  do {
+    GVChanged = false;
+    for (Module::global_iterator GVI = M.global_begin(),
+         E = M.global_end(); GVI != E; ++GVI) {
+      if(TryToHoistGlobalConstantInitializers(GVI)) {
+        Changed |= GVChanged = true;
+        break;
+      }
+    }
+  } while (GVChanged);

No. Iterate until convergence is very bad style in the compiler because we
can't prove that it has less than O(n^2) complexity (or alternatively there
are a few places where it is O(n^2) complexity and we believe that's the
best we can do).

Also, there are a few place where your patch seems unfinished:

+  if (!attemptGlobalInitHoist) return false;
+
 //////////////////////////////////////////////////////////////////////////////

Is this a marker indicating where you wanted to split into separate
functions?

+    if (defBB->size() != 1) {
+      /// Assert?
+    }

Decide this before sending out the patch. If you decide not to assert, you
should probably include a testcase that shows why (ie., that would trip the
assert, demonstrating why the assert is wrong).

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