[PATCH] D54774: [GlobalOpt] Optimize comdat dynamic initializers on Windows

[Reid Kleckner via Phabricator via llvm-commits]

rnk planned changes to this revision.
rnk marked 2 inline comments as done.
rnk added a comment.

It would be nice if globalopt could do a better job on these kinds of things, too:

  inline int foo() { return 42; }
  int bar();
  static int block_globalopt = bar();
  static int g1 = foo();
  static int g2 = foo();
  static int g3 = foo();
  int *arr[] = {&block_globalopt, &g1, &g2, &g3};

C++ requires that these globals are initialized in order, but (I think?) it's UB if any of the globals are accessed before they are constructed. Right now, to provide the order guarantee, we lump all the non-comdat initializers into a `_GLOBAL__sub_I_` function. As a consequence, globalopt is all or nothing: either everything can be optimized, or nothing can be. In this example, because it can't see through `bar`, it won't fire.

Maybe we should consider giving a stronger guarantee for the order in which global_ctors entries run, so that clang can instead emit these all separately.



================
Comment at: llvm/lib/Transforms/Utils/CtorUtils.cpp:116
+  bool AssumeComdatInitializers =
+      Triple(M.getTargetTriple()).isWindowsMSVCEnvironment();
+
----------------
rjmccall wrote:
> rnk wrote:
> > rjmccall wrote:
> > > efriedma wrote:
> > > > If the IR doesn't have enough information to distinguish between the Microsoft and Itanium ABI cases, we should extend the IR to contain more information.  Checking the triple means we're depending on semantic guarantees which are not documented in LangRef, and many not apply to non-C++ languages.
> > > I agree with Eli.  This is clearly a frontend-originated guarantee, not a target-wide guarantee, and should be reflected in the IR.
> > For platforms with comdats, I wanted to push this same idea through the Itanium C++ ABI. It completely eliminates the need for guard variables for initializers of variables with vague linkage, although only if they are hidden, so other DSOs don't try to initialize them.
> > 
> > What do you think of some kind of global named metadata node for this? This is such a complicated guarantee to express, I've tried and failed to write it down concisely three times. `!llvm.assume_comdat_ctors`? `!llvm.comdat_initializers = !{i32 0, i32 1}`? I like "comdat_initializers".
> > For platforms with comdats, I wanted to push this same idea through the Itanium C++ ABI. It completely eliminates the need for guard variables for initializers of variables with vague linkage, although only if they are hidden, so other DSOs don't try to initialize them.
> 
> Okay, but it's still not a target-wide thing, it's a rule of a specific language ABI.  That's a higher level of abstraction than LLVM IR and should be opt-in on a variable-by-variable basis.
> 
> > This is such a complicated guarantee to express, I've tried and failed to write it down concisely three times.
> 
> "If this definition of the global variable is chosen by the linker, there is an associated global constructor function that will also be chosen by the linker, and it is undefined behavior if any other global constructor accesses the variable before the associated constructor runs."  I'd call it `comdat_global_ctor`, but the "comdat" in the name is just suggestive, not a core aspect of the semantics.
I wasn't going to go so far as to make it variable-by-variable, I was just going to make it a global named metadata tuple. I guess a metadata attachment on the global variable being initialized would be OK, since removing just blocks the optimization, which is semantics preserving. I don't like it that much, since I really feel like this optimization should be done as a matter of course. It sounds like you folks think that's the way to go, though?


================
Comment at: llvm/lib/Transforms/Utils/Evaluator.cpp:131
+static bool canUpdateGlobal(GlobalVariable *GV, GlobalVariable *ComdatGV) {
+  return GV == ComdatGV || GV->hasUniqueInitializer();
+}
----------------
efriedma wrote:
> Actually, if we have a ComdatGV, isn't it illegal to update globals where hasUniqueInitializer() is true?  The constructor could be discarded.  For example, something like this:
> 
>     void foo();
>     struct S { S() { foo(); } static S s; };
>     inline S S::s;
>     S *ss = &S::s;
>     int x;
>     void foo(){ x++; }
You're right. Initially I was thinking this definition of `x` will always prevail so modifying it is safe, but the initializer may not prevail, and we could now double-increment `x`. I think it's fixable. Basically, if this is an initializer for something in a comdat, we can only touch globals in that comdat group, so they all go together.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54774/new/

https://reviews.llvm.org/D54774