[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

Thu Oct 20 23:42:55 PDT 2022

ChuanqiXu added a comment.

@dblaikie I am confusing about your concern. For the test coverage example, on the one hand, it is completely different from the case the coverage report was generated in the runtime instead of the compile time. On the other hand, it also provides a `-fprofile-dir` option to control the output directory.

> I'd be happy not to add this until someone comes back with a demonstrated need

Yeah, we have demos. Both https://github.com/ChuanqiXu9/llvm-project/tree/MyP1689/P1689Examples and https://reviews.llvm.org/D135507 are demos for this.

> it doesn't seem like it'd be a major imposition on a build system to copy the file if it needed to to move it into a release tree V the build tree, for instance. And if it did add up to real performance problems, we could add it.

It doesn't make sense. Yes, the build system can provide the same behavior by `mv` commands. But why should we put the effort to the build systems? Especially it won't bring any cost for us to generate the pcm files in the specified positions except an additional flags. And it will be an overhead to move the pcm files at least now. For example, for the following simple module unit:

  module;
  #include <iostream>
  export module Hello;
  export void HelloWorld() {
      std::cout << "Hello World.\n";
  }

the size of the generated `pcm` file is `4.3M`. I know we can say that it is bad the compiler generate a so big pcm files. And it will be great if we can reduce them. However, this the status quo.

> then another for "here's a flag to name the .pcm output file" (not sure if we need that, really, just yet)

It is needed for the build systems to specify the position of pcm output files. In fact, this option is required by @ben.boeckel

> possibly another for "here's an option to write the .pcm file out to a cache directory instead of the .o path or custom .pcm output path" (I've even stronger reservations about that).

To be honest, at least for the current experiments with kitware guys, this is not necessary. However, I think this is helpful for people to (try to) use modules in their current build systems. We know this is not so good. But it is much better than now.

---

After thinking about this more, I found the root divergence between us is about the principle/bar to add a compilation flag sugar (not a functionality but a flag). I feel like your standard for this patch is my standard for the new functionality instead of the compilation flag sugar.

What is a compilation flag sugar to me? My answer is: if we can get the same result without this flag, then this flag is a a compilation flag sugar to me. Then whole of this patch is a compilation sugar to me. How can we get the same result without this patch? For example, for the simple Hello.cppm, we can do the following steps:

  clang++ -std=c++20 Hello.cppm -save-temps -c

then we can see the following files in the current directory:

  Hello.bc  Hello.cppm  Hello.iim  Hello.o  Hello.pcm  Hello.s

the we can remove all the unwanted things, we can get:

  Hello.cppm Hello.pcm

Now we can move the `Hello.pcm` to the so called cache directory.  Then we get the exactly the same result without this patch! This is the reason why I feel this patch is innocent. Since it only reduces the above steps into two flags. And if we take the necessity as the standard for this patch. Then whole of this patch is meaningless. Since the user can still get the results by some shell tricks.

But I think it should be good to do such things for our uses. We shouldn't push such burdens to our users if we can. And it shouldn't bring a burden to us to introduce such two flags.

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

https://reviews.llvm.org/D134267