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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to