[PATCH] D134589: [C++20][Modules] Elide unused guard variables in Itanium ABI module initializers.

[Iain Sandoe via Phabricator via cfe-commits]

iains added a comment.

thanks for the review.



================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:690-695
+  // If we have an empty initializer then we do not want to create a guard var.
+  // 'Empty' needs only to apply to init functions that we call directly, calls
+  // to imported module initializers need not be counted since those will
+  // contain local guards as needed (and might themselves be empty).
+  bool ElideGuard = PrioritizedCXXGlobalInits.empty() && CXXGlobalInits.empty();
+
----------------
dblaikie wrote:
> Usually I'd expect this to be sunk down closer to its usage - any particular reason to have it up here? (not super far away, but seems further away than I'd expect)
Agreed.
The reason was that we build an ordered vector of inits from several potential sources, and I'd been clearing the inputs as they were used. I've now sunk those clears to the end of the function and merged the test as you suggest below.


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:738
+    if (ElideGuard) {
+      CodeGenFunction(*this).GenerateCXXGlobalInitFunc(Fn, ModuleInits);
+    } else {
----------------
dblaikie wrote:
> Could we avoid duplicating this call, by doing something like this:
> ```
> ConstantAddress Guard = ConstantAddress::invalid()
> if (!PrioritizedCXXGlobalInits.empty() || !CXXGlobalInits.empty())
>   Guard = ConstantAddress(new GlobalVariable(...), Int8Ty, GuardAlign);
> CodeGenFunction(*this).GenerateCXXGlobalInitFunc(Fn, ModuleInits, Guard);
> ```
> ?
works for me.


================
Comment at: clang/test/CodeGenCXX/module-initializer-guard-elision.cpp:22-43
+// This module has no global inits and does not import any other module
+//--- O.cpp
+
+export module O;
+
+export int foo ();
+
----------------
dblaikie wrote:
> If it's pretty stable/reliable/not too brittle, maybe worth CHECKing the whole function in these cases (CHECK+CHECK-NEXT etc) to demonstrate they have nothing in them other than the return, or the call+return?
The main thing we care about is that there is no guard variable - however it does no harm to be specific where we can,  For the empty/nearly empty functions, that seems reasonable (done for those).



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134589