[PATCH] D119409: [C++20] [Modules] Remain dynamic initializing internal-linkage variables in module interface unit

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a subscriber: urnathan.
ChuanqiXu added a comment.

In D119409#3400287 <https://reviews.llvm.org/D119409#3400287>, @dblaikie wrote:

> In D119409#3398483 <https://reviews.llvm.org/D119409#3398483>, @ChuanqiXu wrote:
>
>> In D119409#3396690 <https://reviews.llvm.org/D119409#3396690>, @dblaikie wrote:
>>
>>> Not even necessarily then - if you have code like protobufs (large amounts of autogenerated code, much of which you might never call) - or even just libraries where you only use a subset of their features - then you might have more code generated with an inline-function-homing
>>
>> From the perspective of code size, it would be larger in inline-function-homing strategies in cases you described. But I guess it wouldn't hurt in compilation speed, is it?
>
> Depending on optimizations, some amount of compilation time is proportional to code size.

Yeah, I am not strict enough. I mean the compilation time of the importee wouldn't be hurt. The compilation time of the module itself would be more of course. Given the module is imported in many places, it would be a win. But if it is not, then it might be a bad idea.
So the conclusion would be that inline-function-homing wouldn't be a pure win all the time.  I just want to say it might be a win in a lot of cases.

>> For the consideration of code size, I think we could mitigate the problem by thinLTO. (Although we couldn't solve the problem completely since thinLTO couldn't help for *.o, *.a and *.so files.)
>
> Yep. But with ThinLTO we wouldn't need to produce `available_externally` definitions anyway, since ThinLTO can cross-object import anyway.

Yeah. My point is that with the inline-function-homing strategy, we could not produce `available_externally`  functions in the frontend so that we could get a big compilation speedup. In some of my experiments, the speed could be 5x than before : ) It is exciting.

>>> Possibly - there's still the possibility that even without any importing (& just homing into the module object file) that it'll cost more than it benefits due to inline functions that are never called.
>>
>> Yeah, I met the situation before. In a simple case, that the time spent on the deserialization looks more than the time we saved. By from my experience (my experience is not complete though), this kind of cases is relatively small.
>>
>> ---
>>
>> @dblaikie given that the talk involves further optimization, which is beyond the scope of current patch. Would you like to accept this one?
>
> I don't have enough context for the current state of this patch - might be useful to post a summary of the current state?

Yeah. Let me try to give a summary. The intention of the patch is fix the following problem:

  // Hello.cppm
  module;
  #include <cstdio>
  export module Hello;
  export void SayHello()
  {
      std::printf("Hello World.\n");
  
  }
  // main.cpp
  import Hello;
  int main() {
     SayHello();
     return 0;
  }

The program compiled in this way would met a segment fault at run time if it uses libstdc++. The root cause is that libstdc++ uses the constructor of the following static variable to do resources initializations.

  // For construction of filebuffers for cout, cin, cerr, clog et. al.
  static ios_base::Init __ioinit;

And in the original implementation, the static variable (with internal linkage) is considered invisible to the outside so it wouldn't be generated in the PCM (the binary form of AST).

To solve the problem, the patch intends to remain the static variable (or say, variable with internal linkage). Then @urnathan pointed that we should only generate the static variables with dynamic initializer. Then this is the current status of patch.

Although the discuss is very long, almost of them talks about the inline-function-strategy. We talked a lot about that whether it is valid or whether it is useful. But all of them are beyond the scope of the patch : )


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

https://reviews.llvm.org/D119409