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

[Puyan Lotfi]

Thanks everyone, for the feedback. I’ve attached an updated diff for any additional comments (which addresses most of the existing feedback).
I’ll submit another patch once I get the tests integrated into lit.

-Puyan


On Aug 26, 2013, at 1:09 PM, Nick Lewycky <nlewycky at google.com> wrote:

> 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/97c814f3/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: GlobaloptInitHoist2.patch
Type: application/octet-stream
Size: 19934 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130826/97c814f3/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130826/97c814f3/attachment-0001.html>