[PATCH] D125278: [GlobalOpt] Relax the check for ctor priorities

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

Are we sure this is sound? Here is the hard case I can think of: https://gcc.godbolt.org/z/4fYr7jMse

  // follow -DPROBLEM ifdefs
  struct Foo {
      Foo(int x) : x(x) {}
      int x;
  };
  static int bar() { static int id; return ++id; }
  void sideeffect();
  #ifdef PROBLEM
  #define INITP(p) __attribute__((init_priority(p)))
  #define VAL2 (sideeffect(), bar())
  #else
  #define INITP(p) 
  #define VAL2 bar()
  #endif
  INITP(111) Foo gv1(bar());
  INITP(112) Foo gv2(VAL2);
  INITP(113) Foo gv3(bar());

In this case, we have three constructors, ordered by init_priority. The user has set these priorities, and they expect these assignments: `gv1=1`, `gv2=2`, `gv3=3`. If we apply your change, we will get `gv1=1`, `gv2=3`, `gv3=2`, right? `gv2` will be delayed until runtime and happen last. It will not appear as if these ran in the specified order.

The reason we can get away with optimizing the constructors in the default order is that, within priorities, the ordering is not guaranteed.



================
Comment at: llvm/lib/Transforms/Utils/CtorUtils.cpp:111-117
+  // Protect against trying to statically evaluate ctors in the wrong order.
+  // Clang emits the elements of llvm.global_ctors in the correct order (see
+  // CodeGenModule::EmitCXXGlobalInitFunc in clang/lib/CodeGen/CGDeclCXX.cpp),
+  // but the documentation for llvm.global_ctors does not specify if this array
+  // is required to be sorted by priority or not.
+  if (!is_sorted(Priorities))
+    return nullptr;
----------------
I'd prefer not to leave this as an undocumented handshake between Clang and LLVM. I think it's worth going to the effort to parse the priorities out and stable_sort the vector by ascending priority. You should also check what happens during full (fat) LTO. Are global_ctor arrays joined and sorted by priority, or are they left unordered? I suspect they are left unordered, and we want globalopt to work in those scenarios.


================
Comment at: llvm/lib/Transforms/Utils/CtorUtils.cpp:159
       continue;
     }
   }
----------------
It looks to me that we attempt to fold any and all constructors, and we don't stop after we fail to evaluate one of them. I think stopping on the first one that fails would be sound. Effectively, it is "as if" the first constructors ran before the first constructor which cannot be folded.

This is, however, quite unsatisfying since it would prevent folding `gv3` in my linked example, but that's kind of the point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125278