subject:"\[PATCH\] D137058\: \[Driver\] \[Modules\] Support \-fmodule\-output \(1\/2\)"

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2023-01-17 Thread Chuanqi Xu via Phabricator via cfe-commits

ChuanqiXu added a comment.

In D137058#4058836 , @Jake-Egan wrote:

> In D137058#4057424 , @ChuanqiXu 
> wrote:
>
>> In D137058#4056647 , @Jake-Egan 
>> wrote:
>>
>>> Hi, this new test fails on AIX 
>>> https://lab.llvm.org/buildbot/#/builders/214/builds/5351/steps/6/logs/FAIL__Clang__module-output_cppm
>>> Could you take a look?
>>
>> I added `// REQUIRES: x86-registered-target`. Is it still failing?
>
> It is still failing yes, I think it should restrict `system-aix` instead, 
> like you did on windows.

Got it. Could you help to add this? Since I can't test it properly.

> My concern is that when it becomes necessary it won't be apparent - someone 
> will fix (or introduce) a bug in one codepath, unaware of the other similar 
> codepath. Unifying them before that happens is valuable.

Understood. I just felt that the `tools::SplitDebugName`  and 
`GetModuleOutputPath` have more different points than common points.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2023-01-17 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

In D137058#4055275 , @ChuanqiXu wrote:

> In D137058#4050188 , @dblaikie 
> wrote:
>
>> I really don't think this is the right thing to do - the Split DWARF code, 
>> for instance, has support for GPU bundling that's missing in the module file 
>> naming code, which seems likely to be broken & within reason handle-able 
>> today by reusing the Split DWARF code, similarly with the multi-arch 
>> bundling for MachO. But I've tried to explain these things in several ways, 
>> and haven't managed to connect.
>>
>> Carry on.
>
> Thanks for your patient reviewing! We can merge the logics with Split DWARF 
> code someday when we find it necessary.

My concern is that when it becomes necessary it won't be apparent - someone 
will fix (or introduce) a bug in one codepath, unaware of the other similar 
codepath. Unifying them before that happens is valuable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2023-01-17 Thread Jake Egan via Phabricator via cfe-commits

Jake-Egan added a comment.

In D137058#4057424 , @ChuanqiXu wrote:

> In D137058#4056647 , @Jake-Egan 
> wrote:
>
>> Hi, this new test fails on AIX 
>> https://lab.llvm.org/buildbot/#/builders/214/builds/5351/steps/6/logs/FAIL__Clang__module-output_cppm
>> Could you take a look?
>
> I added `// REQUIRES: x86-registered-target`. Is it still failing?

It is still failing yes, I think it should restrict `system-aix` instead, like 
you did on windows.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2023-01-16 Thread Chuanqi Xu via Phabricator via cfe-commits

ChuanqiXu added a comment.

In D137058#4056647 , @Jake-Egan wrote:

> Hi, this new test fails on AIX 
> https://lab.llvm.org/buildbot/#/builders/214/builds/5351/steps/6/logs/FAIL__Clang__module-output_cppm
> Could you take a look?

I added `// REQUIRES: x86-registered-target`. Is it still failing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2023-01-16 Thread Jake Egan via Phabricator via cfe-commits

Jake-Egan added a comment.

Hi, this new test fails on AIX 
https://lab.llvm.org/buildbot/#/builders/214/builds/5351/steps/6/logs/FAIL__Clang__module-output_cppm
Could you take a look?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2023-01-15 Thread Chuanqi Xu via Phabricator via cfe-commits

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGf89327e28bc1: [Driver] [Modules] Support -fmodule-output 
(1/2) (authored by ChuanqiXu).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137058

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/lit.local.cfg
  clang/test/Driver/module-output.cppm

Index: clang/test/Driver/module-output.cppm
===
--- /dev/null
+++ clang/test/Driver/module-output.cppm
@@ -0,0 +1,33 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// Tests that the .pcm file will be generated in the same directory with the specified
+// output and the name of the .pcm file should be the same with the input file.
+// RUN: %clang -std=c++20 %t/Hello.cppm -fmodule-output -c -o %t/output/Hello.o \
+// RUN:   -### 2>&1 | FileCheck %t/Hello.cppm
+//
+// Tests that the output file will be generated in the input directory if the output
+// file is not the corresponding object file.
+// RUN: %clang -std=c++20 %t/Hello.cppm %t/AnotherModule.cppm -fmodule-output -o \
+// RUN:   %t/output/a.out -### 2>&1 | FileCheck  %t/AnotherModule.cppm
+//
+// Tests that clang will reject the command line if it specifies -fmodule-output with
+// multiple archs.
+// RUN: %clang %t/Hello.cppm -fmodule-output -arch i386 -arch x86_64 -### -target \
+// RUN:   x86_64-apple-darwin 2>&1 | FileCheck %t/Hello.cppm -check-prefix=MULTIPLE-ARCH
+
+//--- Hello.cppm
+export module Hello;
+
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/output/Hello.pcm" "-x" "c++" "{{.*}}/Hello.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/output/Hello.o" "-x" "pcm" "{{.*}}/output/Hello.pcm"
+
+// MULTIPLE-ARCH: option '-fmodule-output' can't be used with multiple arch options
+
+//--- AnotherModule.cppm
+export module AnotherModule;
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/Hello.pcm" "-x" "c++" "{{.*}}/Hello.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/Hello-{{.*}}.o" "-x" "pcm" "{{.*}}/Hello.pcm"
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" "AnotherModule.cppm" {{.*}}"-o" "{{.*}}/AnotherModule.pcm" "-x" "c++" "{{.*}}/AnotherModule.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "AnotherModule.cppm" {{.*}}"-o" "{{.*}}/AnotherModule-{{.*}}.o" "-x" "pcm" "{{.*}}/AnotherModule.pcm"
Index: clang/test/Driver/lit.local.cfg
===
--- clang/test/Driver/lit.local.cfg
+++ clang/test/Driver/lit.local.cfg
@@ -1,6 +1,6 @@
 from lit.llvm import llvm_config
 
-config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',
+config.suffixes = ['.c', '.cpp', '.cppm', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',
'.cu', '.rs', '.cl', '.clcpp', '.hip', '.hipi', '.hlsl']
 config.substitutions = list(config.substitutions)
 config.substitutions.insert(0,
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -5553,6 +5553,30 @@
   return C.addTempFile(C.getArgs().MakeArgString(TmpName));
 }
 
+// Calculate the output path of the module file when compiling a module unit
+// with the `-fmodule-output` option specified. The behavior is:
+// - If the output object file of the module unit is specified, the output path
+//   of the module file should be the same with the output object file except
+//   the corresponding suffix. This requires both `-o` and `-c` are specified.
+// - Otherwise, the output path of the module file will be the same with the
+//   input with the corresponding suffix.
+static const char *GetModuleOutputPath(Compilation , const JobAction ,
+   const char *BaseInput) {
+  assert(isa(JA) && JA.getType() == types::TY_ModuleFile &&
+ C.getArgs().hasArg(options::OPT_fmodule_output));
+
+  SmallString<64> OutputPath;
+  Arg *FinalOutput = C.getArgs().getLastArg(options::OPT_o);
+  if (FinalOutput && C.getArgs().hasArg(options::OPT_c))
+OutputPath = FinalOutput->getValue();
+  else
+OutputPath = BaseInput;
+
+  const char *Extension = types::getTypeTempSuffix(JA.getType());
+  llvm::sys::path::replace_extension(OutputPath, Extension);
+  return C.addResultFile(C.getArgs().MakeArgString(OutputPath.c_str()), );
+}
+
 const char *Driver::GetNamedOutputPath(Compilation , const JobAction ,
const char *BaseInput,
StringRef OrigBoundArch, bool AtTopLevel,

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2023-01-15 Thread Chuanqi Xu via Phabricator via cfe-commits

ChuanqiXu added a comment.

In D137058#4050188 , @dblaikie wrote:

> I really don't think this is the right thing to do - the Split DWARF code, 
> for instance, has support for GPU bundling that's missing in the module file 
> naming code, which seems likely to be broken & within reason handle-able 
> today by reusing the Split DWARF code, similarly with the multi-arch bundling 
> for MachO. But I've tried to explain these things in several ways, and 
> haven't managed to connect.
>
> Carry on.

Thanks for your patient reviewing! We can merge the logics with Split DWARF 
code someday when we find it necessary.


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2023-01-12 Thread David Blaikie via Phabricator via cfe-commits

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

I really don't think this is the right thing to do - the Split DWARF code, for 
instance, has support for GPU bundling that's missing in the module file naming 
code, which seems likely to be broken & within reason handle-able today by 
reusing the Split DWARF code, similarly with the multi-arch bundling for MachO. 
But I've tried to explain these things in several ways, and haven't managed to 
connect.

Carry on.


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2023-01-12 Thread Chuanqi Xu via Phabricator via cfe-commits

ChuanqiXu marked an inline comment as done.
ChuanqiXu added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:5736
+  C.getArgs().hasArg(options::OPT_fmodule_output) &&
+  C.getArgs().hasArg(options::OPT_o)) {
+SmallString<128> OutputPath;

h-vetinari wrote:
> dblaikie wrote:
> > ChuanqiXu wrote:
> > > dblaikie wrote:
> > > > tahonermann wrote:
> > > > > dblaikie wrote:
> > > > > > ChuanqiXu wrote:
> > > > > > > dblaikie wrote:
> > > > > > > > Is there some way we can avoid this (`-fmodule-output  -o ...`) 
> > > > > > > > being a special case? It'd be good if we could use a single 
> > > > > > > > common implementation regardless of whether `-o` is used (ie: 
> > > > > > > > Figure out the output file name (whether it's implicitly 
> > > > > > > > determined by clang, in the absence of `-o`, or explicitly 
> > > > > > > > provided by the user through `-o`) and then replace the suffix 
> > > > > > > > with `pcm`)?
> > > > > > > I guess we can't do it or we can't do it easily. Otherwise the 
> > > > > > > .pcm file will always lives in the same directory with the input 
> > > > > > > file.
> > > > > > > I guess we can't do it or we can't do it easily. Otherwise the 
> > > > > > > .pcm file will always lives in the same directory with the input 
> > > > > > > file.
> > > > > > 
> > > > > > I don't follow/understand. What I mean is, I'd like it, if 
> > > > > > possible, this was implemented by something like "find the path for 
> > > > > > the .o file output, then replace the extension with .pcm".
> > > > > > 
> > > > > > I worry atht code like this that recomputes something similar to 
> > > > > > the object output path risks getting out of sync with the actual 
> > > > > > object path.
> > > > > That is certainly a valid concern and I agree it would be better if 
> > > > > the shared output path is computed exactly once. If that would 
> > > > > require significant change, then tests to ensure consistent behavior 
> > > > > would be the next best thing. I'm not sure what infrastructure is in 
> > > > > place for validation of output file locations though.
> > > > Well, I guess the Split DWARF file naming isn't fundamentally better - 
> > > > it looks at the OPT_O argument directly too:
> > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/CommonArgs.cpp#L1250
> > > > 
> > > > Perhaps we could at least reuse this same logic - make it a reusable 
> > > > function in some way? (for instance, it checks `-c` too, which seems 
> > > > relevant - we wouldn't want to name the .pcm after the linked 
> > > > executable name, right?
> > > > 
> > > > Perhaps we could at least reuse this same logic - make it a reusable 
> > > > function in some way? 
> > > 
> > > Done. It looks better now.
> > > 
> > > > (for instance, it checks -c too, which seems relevant - we wouldn't 
> > > > want to name the .pcm after the linked executable name, right?
> > > 
> > > Oh, right. Although the previous conclusion is that if `-o` is specified, 
> > > the .pcm file should be in the same dir with the output. But it is indeed 
> > > weird that the .pcm file are related to the linked executable if we 
> > > thought it deeper. 
> > Ah, I was hoping it could reuse the same code, rather than duplicate it - 
> > any chance it could be refactored into a common implementation between 
> > Split DWARF and modules?
> > Ah, I was hoping it could reuse the same code, rather than duplicate it - 
> > any chance it could be refactored into a common implementation between 
> > Split DWARF and modules?
> 
> Could we uncouple this clean-up from landing the patches before the LLVM 16 
> branch? A trivial refactor might even still be backportable, but the whole 
> series will be more challenging.
> Ah, I was hoping it could reuse the same code, rather than duplicate it - any 
> chance it could be refactored into a common implementation between Split 
> DWARF and modules?

I feel it is an overkill to merge these 2 logics. It will become harder to read 
and modify the logics after we merge them. I feel better with the current 
implementation since it has fewer code dependencies and its complexity is low.


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2023-01-12 Thread Chuanqi Xu via Phabricator via cfe-commits

ChuanqiXu updated this revision to Diff 488836.
ChuanqiXu marked an inline comment as done.
ChuanqiXu added a comment.

Address comments: fix a typo.


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

https://reviews.llvm.org/D137058

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/lit.local.cfg
  clang/test/Driver/module-output.cppm

Index: clang/test/Driver/module-output.cppm
===
--- /dev/null
+++ clang/test/Driver/module-output.cppm
@@ -0,0 +1,33 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// Tests that the .pcm file will be generated in the same directory with the specified
+// output and the name of the .pcm file should be the same with the input file.
+// RUN: %clang -std=c++20 %t/Hello.cppm -fmodule-output -c -o %t/output/Hello.o \
+// RUN:   -### 2>&1 | FileCheck %t/Hello.cppm
+//
+// Tests that the output file will be generated in the input directory if the output
+// file is not the corresponding object file.
+// RUN: %clang -std=c++20 %t/Hello.cppm %t/AnotherModule.cppm -fmodule-output -o \
+// RUN:   %t/output/a.out -### 2>&1 | FileCheck  %t/AnotherModule.cppm
+//
+// Tests that clang will reject the command line if it specifies -fmodule-output with
+// multiple archs.
+// RUN: %clang %t/Hello.cppm -fmodule-output -arch i386 -arch x86_64 -### -target \
+// RUN:   x86_64-apple-darwin 2>&1 | FileCheck %t/Hello.cppm -check-prefix=MULTIPLE-ARCH
+
+//--- Hello.cppm
+export module Hello;
+
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/output/Hello.pcm" "-x" "c++" "{{.*}}/Hello.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/output/Hello.o" "-x" "pcm" "{{.*}}/output/Hello.pcm"
+
+// MULTIPLE-ARCH: option '-fmodule-output' can't be used with multiple arch options
+
+//--- AnotherModule.cppm
+export module AnotherModule;
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/Hello.pcm" "-x" "c++" "{{.*}}/Hello.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/Hello-{{.*}}.o" "-x" "pcm" "{{.*}}/Hello.pcm"
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" "AnotherModule.cppm" {{.*}}"-o" "{{.*}}/AnotherModule.pcm" "-x" "c++" "{{.*}}/AnotherModule.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "AnotherModule.cppm" {{.*}}"-o" "{{.*}}/AnotherModule-{{.*}}.o" "-x" "pcm" "{{.*}}/AnotherModule.pcm"
Index: clang/test/Driver/lit.local.cfg
===
--- clang/test/Driver/lit.local.cfg
+++ clang/test/Driver/lit.local.cfg
@@ -1,6 +1,6 @@
 from lit.llvm import llvm_config
 
-config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',
+config.suffixes = ['.c', '.cpp', '.cppm', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',
'.cu', '.rs', '.cl', '.clcpp', '.hip', '.hipi', '.hlsl']
 config.substitutions = list(config.substitutions)
 config.substitutions.insert(0,
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -5553,6 +5553,30 @@
   return C.addTempFile(C.getArgs().MakeArgString(TmpName));
 }
 
+// Calculate the output path of the module file when compiling a module unit
+// with the `-fmodule-output` option specified. The behavior is:
+// - If the output object file of the module unit is specified, the output path
+//   of the module file should be the same with the output object file except
+//   the corresponding suffix. This requires both `-o` and `-c` are specified.
+// - Otherwise, the output path of the module file will be the same with the
+//   input with the corresponding suffix.
+static const char *GetModuleOutputPath(Compilation , const JobAction ,
+   const char *BaseInput) {
+  assert(isa(JA) && JA.getType() == types::TY_ModuleFile &&
+ C.getArgs().hasArg(options::OPT_fmodule_output));
+
+  SmallString<64> OutputPath;
+  Arg *FinalOutput = C.getArgs().getLastArg(options::OPT_o);
+  if (FinalOutput && C.getArgs().hasArg(options::OPT_c))
+OutputPath = FinalOutput->getValue();
+  else
+OutputPath = BaseInput;
+
+  const char *Extension = types::getTypeTempSuffix(JA.getType());
+  llvm::sys::path::replace_extension(OutputPath, Extension);
+  return C.addResultFile(C.getArgs().MakeArgString(OutputPath.c_str()), );
+}
+
 const char *Driver::GetNamedOutputPath(Compilation , const JobAction ,
const char *BaseInput,
StringRef OrigBoundArch, bool AtTopLevel,
@@ -5609,6 +5633,16 @@
 );
   }
 
+  if (MultipleArchs && C.getArgs().hasArg(options::OPT_fmodule_output))
+

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2023-01-12 Thread H. Vetinari via Phabricator via cfe-commits

h-vetinari added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:5639-5640
+
+  // If we're emitting a module output with the speicifed option
+  // `-fmodule-output`.
+  if (!AtTopLevel && isa(JA) &&





Comment at: clang/lib/Driver/Driver.cpp:5736
+  C.getArgs().hasArg(options::OPT_fmodule_output) &&
+  C.getArgs().hasArg(options::OPT_o)) {
+SmallString<128> OutputPath;

dblaikie wrote:
> ChuanqiXu wrote:
> > dblaikie wrote:
> > > tahonermann wrote:
> > > > dblaikie wrote:
> > > > > ChuanqiXu wrote:
> > > > > > dblaikie wrote:
> > > > > > > Is there some way we can avoid this (`-fmodule-output  -o ...`) 
> > > > > > > being a special case? It'd be good if we could use a single 
> > > > > > > common implementation regardless of whether `-o` is used (ie: 
> > > > > > > Figure out the output file name (whether it's implicitly 
> > > > > > > determined by clang, in the absence of `-o`, or explicitly 
> > > > > > > provided by the user through `-o`) and then replace the suffix 
> > > > > > > with `pcm`)?
> > > > > > I guess we can't do it or we can't do it easily. Otherwise the .pcm 
> > > > > > file will always lives in the same directory with the input file.
> > > > > > I guess we can't do it or we can't do it easily. Otherwise the .pcm 
> > > > > > file will always lives in the same directory with the input file.
> > > > > 
> > > > > I don't follow/understand. What I mean is, I'd like it, if possible, 
> > > > > this was implemented by something like "find the path for the .o file 
> > > > > output, then replace the extension with .pcm".
> > > > > 
> > > > > I worry atht code like this that recomputes something similar to the 
> > > > > object output path risks getting out of sync with the actual object 
> > > > > path.
> > > > That is certainly a valid concern and I agree it would be better if the 
> > > > shared output path is computed exactly once. If that would require 
> > > > significant change, then tests to ensure consistent behavior would be 
> > > > the next best thing. I'm not sure what infrastructure is in place for 
> > > > validation of output file locations though.
> > > Well, I guess the Split DWARF file naming isn't fundamentally better - it 
> > > looks at the OPT_O argument directly too:
> > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/CommonArgs.cpp#L1250
> > > 
> > > Perhaps we could at least reuse this same logic - make it a reusable 
> > > function in some way? (for instance, it checks `-c` too, which seems 
> > > relevant - we wouldn't want to name the .pcm after the linked executable 
> > > name, right?
> > > 
> > > Perhaps we could at least reuse this same logic - make it a reusable 
> > > function in some way? 
> > 
> > Done. It looks better now.
> > 
> > > (for instance, it checks -c too, which seems relevant - we wouldn't want 
> > > to name the .pcm after the linked executable name, right?
> > 
> > Oh, right. Although the previous conclusion is that if `-o` is specified, 
> > the .pcm file should be in the same dir with the output. But it is indeed 
> > weird that the .pcm file are related to the linked executable if we thought 
> > it deeper. 
> Ah, I was hoping it could reuse the same code, rather than duplicate it - any 
> chance it could be refactored into a common implementation between Split 
> DWARF and modules?
> Ah, I was hoping it could reuse the same code, rather than duplicate it - any 
> chance it could be refactored into a common implementation between Split 
> DWARF and modules?

Could we uncouple this clean-up from landing the patches before the LLVM 16 
branch? A trivial refactor might even still be backportable, but the whole 
series will be more challenging.


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2023-01-12 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:5736
+  C.getArgs().hasArg(options::OPT_fmodule_output) &&
+  C.getArgs().hasArg(options::OPT_o)) {
+SmallString<128> OutputPath;

ChuanqiXu wrote:
> dblaikie wrote:
> > tahonermann wrote:
> > > dblaikie wrote:
> > > > ChuanqiXu wrote:
> > > > > dblaikie wrote:
> > > > > > Is there some way we can avoid this (`-fmodule-output  -o ...`) 
> > > > > > being a special case? It'd be good if we could use a single common 
> > > > > > implementation regardless of whether `-o` is used (ie: Figure out 
> > > > > > the output file name (whether it's implicitly determined by clang, 
> > > > > > in the absence of `-o`, or explicitly provided by the user through 
> > > > > > `-o`) and then replace the suffix with `pcm`)?
> > > > > I guess we can't do it or we can't do it easily. Otherwise the .pcm 
> > > > > file will always lives in the same directory with the input file.
> > > > > I guess we can't do it or we can't do it easily. Otherwise the .pcm 
> > > > > file will always lives in the same directory with the input file.
> > > > 
> > > > I don't follow/understand. What I mean is, I'd like it, if possible, 
> > > > this was implemented by something like "find the path for the .o file 
> > > > output, then replace the extension with .pcm".
> > > > 
> > > > I worry atht code like this that recomputes something similar to the 
> > > > object output path risks getting out of sync with the actual object 
> > > > path.
> > > That is certainly a valid concern and I agree it would be better if the 
> > > shared output path is computed exactly once. If that would require 
> > > significant change, then tests to ensure consistent behavior would be the 
> > > next best thing. I'm not sure what infrastructure is in place for 
> > > validation of output file locations though.
> > Well, I guess the Split DWARF file naming isn't fundamentally better - it 
> > looks at the OPT_O argument directly too:
> > https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/CommonArgs.cpp#L1250
> > 
> > Perhaps we could at least reuse this same logic - make it a reusable 
> > function in some way? (for instance, it checks `-c` too, which seems 
> > relevant - we wouldn't want to name the .pcm after the linked executable 
> > name, right?
> > 
> > Perhaps we could at least reuse this same logic - make it a reusable 
> > function in some way? 
> 
> Done. It looks better now.
> 
> > (for instance, it checks -c too, which seems relevant - we wouldn't want to 
> > name the .pcm after the linked executable name, right?
> 
> Oh, right. Although the previous conclusion is that if `-o` is specified, the 
> .pcm file should be in the same dir with the output. But it is indeed weird 
> that the .pcm file are related to the linked executable if we thought it 
> deeper. 
Ah, I was hoping it could reuse the same code, rather than duplicate it - any 
chance it could be refactored into a common implementation between Split DWARF 
and modules?


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2023-01-11 Thread Chuanqi Xu via Phabricator via cfe-commits

ChuanqiXu marked an inline comment as done.
ChuanqiXu added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:5736
+  C.getArgs().hasArg(options::OPT_fmodule_output) &&
+  C.getArgs().hasArg(options::OPT_o)) {
+SmallString<128> OutputPath;

dblaikie wrote:
> tahonermann wrote:
> > dblaikie wrote:
> > > ChuanqiXu wrote:
> > > > dblaikie wrote:
> > > > > Is there some way we can avoid this (`-fmodule-output  -o ...`) being 
> > > > > a special case? It'd be good if we could use a single common 
> > > > > implementation regardless of whether `-o` is used (ie: Figure out the 
> > > > > output file name (whether it's implicitly determined by clang, in the 
> > > > > absence of `-o`, or explicitly provided by the user through `-o`) and 
> > > > > then replace the suffix with `pcm`)?
> > > > I guess we can't do it or we can't do it easily. Otherwise the .pcm 
> > > > file will always lives in the same directory with the input file.
> > > > I guess we can't do it or we can't do it easily. Otherwise the .pcm 
> > > > file will always lives in the same directory with the input file.
> > > 
> > > I don't follow/understand. What I mean is, I'd like it, if possible, this 
> > > was implemented by something like "find the path for the .o file output, 
> > > then replace the extension with .pcm".
> > > 
> > > I worry atht code like this that recomputes something similar to the 
> > > object output path risks getting out of sync with the actual object path.
> > That is certainly a valid concern and I agree it would be better if the 
> > shared output path is computed exactly once. If that would require 
> > significant change, then tests to ensure consistent behavior would be the 
> > next best thing. I'm not sure what infrastructure is in place for 
> > validation of output file locations though.
> Well, I guess the Split DWARF file naming isn't fundamentally better - it 
> looks at the OPT_O argument directly too:
> https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/CommonArgs.cpp#L1250
> 
> Perhaps we could at least reuse this same logic - make it a reusable function 
> in some way? (for instance, it checks `-c` too, which seems relevant - we 
> wouldn't want to name the .pcm after the linked executable name, right?
> 
> Perhaps we could at least reuse this same logic - make it a reusable function 
> in some way? 

Done. It looks better now.

> (for instance, it checks -c too, which seems relevant - we wouldn't want to 
> name the .pcm after the linked executable name, right?

Oh, right. Although the previous conclusion is that if `-o` is specified, the 
.pcm file should be in the same dir with the output. But it is indeed weird 
that the .pcm file are related to the linked executable if we thought it 
deeper. 


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2023-01-11 Thread Chuanqi Xu via Phabricator via cfe-commits

ChuanqiXu updated this revision to Diff 488455.
ChuanqiXu added a comment.

Address comments:

- Extract the logic to compute the output path of `-fmodule-output` to a 
reusable function.


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

https://reviews.llvm.org/D137058

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/lit.local.cfg
  clang/test/Driver/module-output.cppm

Index: clang/test/Driver/module-output.cppm
===
--- /dev/null
+++ clang/test/Driver/module-output.cppm
@@ -0,0 +1,33 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// Tests that the .pcm file will be generated in the same directory with the specified
+// output and the name of the .pcm file should be the same with the input file.
+// RUN: %clang -std=c++20 %t/Hello.cppm -fmodule-output -c -o %t/output/Hello.o \
+// RUN:   -### 2>&1 | FileCheck %t/Hello.cppm
+//
+// Tests that the output file will be generated in the input directory if the output
+// file is not the corresponding object file.
+// RUN: %clang -std=c++20 %t/Hello.cppm %t/AnotherModule.cppm -fmodule-output -o \
+// RUN:   %t/output/a.out -### 2>&1 | FileCheck  %t/AnotherModule.cppm
+//
+// Tests that clang will reject the command line if it specifies -fmodule-output with
+// multiple archs.
+// RUN: %clang %t/Hello.cppm -fmodule-output -arch i386 -arch x86_64 -### -target \
+// RUN:   x86_64-apple-darwin 2>&1 | FileCheck %t/Hello.cppm -check-prefix=MULTIPLE-ARCH
+
+//--- Hello.cppm
+export module Hello;
+
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/output/Hello.pcm" "-x" "c++" "{{.*}}/Hello.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/output/Hello.o" "-x" "pcm" "{{.*}}/output/Hello.pcm"
+
+// MULTIPLE-ARCH: option '-fmodule-output' can't be used with multiple arch options
+
+//--- AnotherModule.cppm
+export module AnotherModule;
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/Hello.pcm" "-x" "c++" "{{.*}}/Hello.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/Hello-{{.*}}.o" "-x" "pcm" "{{.*}}/Hello.pcm"
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" "AnotherModule.cppm" {{.*}}"-o" "{{.*}}/AnotherModule.pcm" "-x" "c++" "{{.*}}/AnotherModule.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "AnotherModule.cppm" {{.*}}"-o" "{{.*}}/AnotherModule-{{.*}}.o" "-x" "pcm" "{{.*}}/AnotherModule.pcm"
Index: clang/test/Driver/lit.local.cfg
===
--- clang/test/Driver/lit.local.cfg
+++ clang/test/Driver/lit.local.cfg
@@ -1,6 +1,6 @@
 from lit.llvm import llvm_config
 
-config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',
+config.suffixes = ['.c', '.cpp', '.cppm', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',
'.cu', '.rs', '.cl', '.clcpp', '.hip', '.hipi', '.hlsl']
 config.substitutions = list(config.substitutions)
 config.substitutions.insert(0,
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -5553,6 +5553,30 @@
   return C.addTempFile(C.getArgs().MakeArgString(TmpName));
 }
 
+// Calculate the output path of the module file when compiling a module unit
+// with the `-fmodule-output` option specified. The behavior is:
+// - If the output object file of the module unit is specified, the output path
+//   of the module file should be the same with the output object file except
+//   the corresponding suffix. This requires both `-o` and `-c` are specified.
+// - Otherwise, the output path of the module file will be the same with the
+//   input with the corresponding suffix.
+static const char *GetModuleOutputPath(Compilation , const JobAction ,
+   const char *BaseInput) {
+  assert(isa(JA) && JA.getType() == types::TY_ModuleFile &&
+ C.getArgs().hasArg(options::OPT_fmodule_output));
+
+  SmallString<64> OutputPath;
+  Arg *FinalOutput = C.getArgs().getLastArg(options::OPT_o);
+  if (FinalOutput && C.getArgs().hasArg(options::OPT_c))
+OutputPath = FinalOutput->getValue();
+  else
+OutputPath = BaseInput;
+
+  const char *Extension = types::getTypeTempSuffix(JA.getType());
+  llvm::sys::path::replace_extension(OutputPath, Extension);
+  return C.addResultFile(C.getArgs().MakeArgString(OutputPath.c_str()), );
+}
+
 const char *Driver::GetNamedOutputPath(Compilation , const JobAction ,
const char *BaseInput,
StringRef OrigBoundArch, bool AtTopLevel,
@@ -5609,6 +5633,16 @@
 );
   }
 
+  if (MultipleArchs &&

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2023-01-11 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:5736
+  C.getArgs().hasArg(options::OPT_fmodule_output) &&
+  C.getArgs().hasArg(options::OPT_o)) {
+SmallString<128> OutputPath;

tahonermann wrote:
> dblaikie wrote:
> > ChuanqiXu wrote:
> > > dblaikie wrote:
> > > > Is there some way we can avoid this (`-fmodule-output  -o ...`) being a 
> > > > special case? It'd be good if we could use a single common 
> > > > implementation regardless of whether `-o` is used (ie: Figure out the 
> > > > output file name (whether it's implicitly determined by clang, in the 
> > > > absence of `-o`, or explicitly provided by the user through `-o`) and 
> > > > then replace the suffix with `pcm`)?
> > > I guess we can't do it or we can't do it easily. Otherwise the .pcm file 
> > > will always lives in the same directory with the input file.
> > > I guess we can't do it or we can't do it easily. Otherwise the .pcm file 
> > > will always lives in the same directory with the input file.
> > 
> > I don't follow/understand. What I mean is, I'd like it, if possible, this 
> > was implemented by something like "find the path for the .o file output, 
> > then replace the extension with .pcm".
> > 
> > I worry atht code like this that recomputes something similar to the object 
> > output path risks getting out of sync with the actual object path.
> That is certainly a valid concern and I agree it would be better if the 
> shared output path is computed exactly once. If that would require 
> significant change, then tests to ensure consistent behavior would be the 
> next best thing. I'm not sure what infrastructure is in place for validation 
> of output file locations though.
Well, I guess the Split DWARF file naming isn't fundamentally better - it looks 
at the OPT_O argument directly too:
https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/CommonArgs.cpp#L1250

Perhaps we could at least reuse this same logic - make it a reusable function 
in some way? (for instance, it checks `-c` too, which seems relevant - we 
wouldn't want to name the .pcm after the linked executable name, right?



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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2023-01-11 Thread Tom Honermann via Phabricator via cfe-commits

tahonermann added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:5736
+  C.getArgs().hasArg(options::OPT_fmodule_output) &&
+  C.getArgs().hasArg(options::OPT_o)) {
+SmallString<128> OutputPath;

dblaikie wrote:
> ChuanqiXu wrote:
> > dblaikie wrote:
> > > Is there some way we can avoid this (`-fmodule-output  -o ...`) being a 
> > > special case? It'd be good if we could use a single common implementation 
> > > regardless of whether `-o` is used (ie: Figure out the output file name 
> > > (whether it's implicitly determined by clang, in the absence of `-o`, or 
> > > explicitly provided by the user through `-o`) and then replace the suffix 
> > > with `pcm`)?
> > I guess we can't do it or we can't do it easily. Otherwise the .pcm file 
> > will always lives in the same directory with the input file.
> > I guess we can't do it or we can't do it easily. Otherwise the .pcm file 
> > will always lives in the same directory with the input file.
> 
> I don't follow/understand. What I mean is, I'd like it, if possible, this was 
> implemented by something like "find the path for the .o file output, then 
> replace the extension with .pcm".
> 
> I worry atht code like this that recomputes something similar to the object 
> output path risks getting out of sync with the actual object path.
That is certainly a valid concern and I agree it would be better if the shared 
output path is computed exactly once. If that would require significant change, 
then tests to ensure consistent behavior would be the next best thing. I'm not 
sure what infrastructure is in place for validation of output file locations 
though.


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2023-01-11 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:5736
+  C.getArgs().hasArg(options::OPT_fmodule_output) &&
+  C.getArgs().hasArg(options::OPT_o)) {
+SmallString<128> OutputPath;

ChuanqiXu wrote:
> dblaikie wrote:
> > Is there some way we can avoid this (`-fmodule-output  -o ...`) being a 
> > special case? It'd be good if we could use a single common implementation 
> > regardless of whether `-o` is used (ie: Figure out the output file name 
> > (whether it's implicitly determined by clang, in the absence of `-o`, or 
> > explicitly provided by the user through `-o`) and then replace the suffix 
> > with `pcm`)?
> I guess we can't do it or we can't do it easily. Otherwise the .pcm file will 
> always lives in the same directory with the input file.
> I guess we can't do it or we can't do it easily. Otherwise the .pcm file will 
> always lives in the same directory with the input file.

I don't follow/understand. What I mean is, I'd like it, if possible, this was 
implemented by something like "find the path for the .o file output, then 
replace the extension with .pcm".

I worry atht code like this that recomputes something similar to the object 
output path risks getting out of sync with the actual object path.


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2023-01-10 Thread Chuanqi Xu via Phabricator via cfe-commits

ChuanqiXu marked an inline comment as done.
ChuanqiXu added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:5736
+  C.getArgs().hasArg(options::OPT_fmodule_output) &&
+  C.getArgs().hasArg(options::OPT_o)) {
+SmallString<128> OutputPath;

dblaikie wrote:
> Is there some way we can avoid this (`-fmodule-output  -o ...`) being a 
> special case? It'd be good if we could use a single common implementation 
> regardless of whether `-o` is used (ie: Figure out the output file name 
> (whether it's implicitly determined by clang, in the absence of `-o`, or 
> explicitly provided by the user through `-o`) and then replace the suffix 
> with `pcm`)?
I guess we can't do it or we can't do it easily. Otherwise the .pcm file will 
always lives in the same directory with the input file.


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2023-01-10 Thread Chuanqi Xu via Phabricator via cfe-commits

ChuanqiXu updated this revision to Diff 488057.
ChuanqiXu added a comment.

Address comments: merge the conditions.


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

https://reviews.llvm.org/D137058

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/lit.local.cfg
  clang/test/Driver/module-output.cppm

Index: clang/test/Driver/module-output.cppm
===
--- /dev/null
+++ clang/test/Driver/module-output.cppm
@@ -0,0 +1,33 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// Tests that the .pcm file will be generated in the same directory with the specified
+// output and the name of the .pcm file should be the same with the input file.
+// RUN: %clang -std=c++20 %t/Hello.cppm -fmodule-output -c -o %t/output/Hello.o \
+// RUN:   -### 2>&1 | FileCheck %t/Hello.cppm
+//
+// Tests that the output file will be generated in the output directory when we
+// specified multiple input files.
+// RUN: %clang -std=c++20 %t/Hello.cppm %t/AnotherModule.cppm -fmodule-output -o \
+// RUN:   %t/output/a.out -### 2>&1 | FileCheck  %t/AnotherModule.cppm
+//
+// Tests that clang will reject the command line if it specifies -fmodule-output with
+// multiple archs.
+// RUN: %clang %t/Hello.cppm -fmodule-output -arch i386 -arch x86_64 -### -target \
+// RUN:   x86_64-apple-darwin 2>&1 | FileCheck %t/Hello.cppm -check-prefix=MULTIPLE-ARCH
+
+//--- Hello.cppm
+export module Hello;
+
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/output/Hello.pcm" "-x" "c++" "{{.*}}/Hello.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/output/Hello.o" "-x" "pcm" "{{.*}}/output/Hello.pcm"
+
+// MULTIPLE-ARCH: option '-fmodule-output' can't be used with multiple arch options
+
+//--- AnotherModule.cppm
+export module AnotherModule;
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/output/Hello.pcm" "-x" "c++" "{{.*}}/Hello.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/Hello-{{.*}}.o" "-x" "pcm" "{{.*}}/output/Hello.pcm"
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" "AnotherModule.cppm" {{.*}}"-o" "{{.*}}/output/AnotherModule.pcm" "-x" "c++" "{{.*}}/AnotherModule.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "AnotherModule.cppm" {{.*}}"-o" "{{.*}}/AnotherModule-{{.*}}.o" "-x" "pcm" "{{.*}}/output/AnotherModule.pcm"
Index: clang/test/Driver/lit.local.cfg
===
--- clang/test/Driver/lit.local.cfg
+++ clang/test/Driver/lit.local.cfg
@@ -1,6 +1,6 @@
 from lit.llvm import llvm_config
 
-config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',
+config.suffixes = ['.c', '.cpp', '.cppm', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',
'.cu', '.rs', '.cl', '.clcpp', '.hip', '.hlsl']
 config.substitutions = list(config.substitutions)
 config.substitutions.insert(0,
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -5566,9 +5566,21 @@
 );
   }
 
+  if (MultipleArchs && C.getArgs().hasArg(options::OPT_fmodule_output))
+Diag(clang::diag::err_drv_module_output_with_multiple_arch);
+
+  // If we're emitting a module output with the speicifed option
+  // `-fmodule-output`.
+  bool EmittingModuleOutput = !AtTopLevel && isa(JA) &&
+  JA.getType() == types::TY_ModuleFile &&
+  C.getArgs().hasArg(options::OPT_fmodule_output);
+
   // Output to a temporary file?
   if ((!AtTopLevel && !isSaveTempsEnabled() &&
-   !C.getArgs().hasArg(options::OPT__SLASH_Fo)) ||
+   !C.getArgs().hasArg(options::OPT__SLASH_Fo) &&
+   // We don't generate module file to temporary file if
+   // we specified `-fmodule-output`
+   !EmittingModuleOutput) ||
   CCGenDiagnostics) {
 StringRef Name = llvm::sys::path::filename(BaseInput);
 std::pair Split = Name.split('.');
@@ -5725,9 +5737,24 @@
 else
   llvm::sys::path::append(BasePath, NamedOutput);
 return C.addResultFile(C.getArgs().MakeArgString(BasePath.c_str()), );
-  } else {
-return C.addResultFile(NamedOutput, );
   }
+
+  // When we specified `-fmodule-output` and `-o`, the module file
+  // will be generated into the same directory with the output file
+  // and the name of the module file would be the input file with the
+  // new suffix '.pcm'.
+  if (EmittingModuleOutput && C.getArgs().hasArg(options::OPT_o)) {
+SmallString<128> OutputPath;
+OutputPath = C.getArgs().getLastArg(options::OPT_o)->getValue();
+llvm::sys::path::remove_filename(OutputPath);
+if (OutputPath.empty())
+

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2023-01-10 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:5733-5735
+  if (!AtTopLevel && isa(JA) &&
+  JA.getType() == types::TY_ModuleFile &&
+  C.getArgs().hasArg(options::OPT_fmodule_output) &&

These conditions might be shared with the previous condition that tests the 
opposite?



Comment at: clang/lib/Driver/Driver.cpp:5736
+  C.getArgs().hasArg(options::OPT_fmodule_output) &&
+  C.getArgs().hasArg(options::OPT_o)) {
+SmallString<128> OutputPath;

Is there some way we can avoid this (`-fmodule-output  -o ...`) being a special 
case? It'd be good if we could use a single common implementation regardless of 
whether `-o` is used (ie: Figure out the output file name (whether it's 
implicitly determined by clang, in the absence of `-o`, or explicitly provided 
by the user through `-o`) and then replace the suffix with `pcm`)?


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2023-01-09 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

In D137058#4030331 , @ChuanqiXu wrote:

> @dblaikie would you like to take a look at this?

Yep, this is still on my radar - sorry for the delays. I'll get to it.


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2023-01-08 Thread H. Vetinari via Phabricator via cfe-commits

h-vetinari added a comment.

> BTW, the series of clang-scan-deps patch (https://reviews.llvm.org/D139168) 
> is also necessary [...]

Yes, I meant both those series.

> [...] but I am not sure if we can land them in time. I guess it may be 
> possible to backport these patches to 16.x since it is relatively important?

Might make sense tagging the release managers if things get tight - perhaps 
it's possible. E.g. ranges for LLVM 15 was also finished after the branch.


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2023-01-08 Thread Chuanqi Xu via Phabricator via cfe-commits

ChuanqiXu added a comment.

In D137058#4033825 , @h-vetinari 
wrote:

> Without undue haste, I just wanted to comment from the peanut gallery that it 
> would be amazing if the patches that are necessary for CMake + Clang to use 
> C++ modules would make the cut-off for LLVM 16 that's coming up around the 
> 24th of January.

Thanks for the note. I'll land this patch in the next week if no further 
comments come in. BTW, the series of clang-scan-deps patch 
(https://reviews.llvm.org/D139168) is also necessary but I am not sure if we 
can land them in time. I guess it may be possible to backport these patches to 
16.x since it is relatively important?

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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2023-01-07 Thread H. Vetinari via Phabricator via cfe-commits

h-vetinari added a comment.

Without undue haste, I just wanted to comment from the peanut gallery that it 
would be amazing if the patches that are necessary for CMake + Clang to use C++ 
modules would make the cut-off for LLVM 16 that's coming up around the 24th of 
January.


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2023-01-05 Thread Chuanqi Xu via Phabricator via cfe-commits

ChuanqiXu added a comment.

@dblaikie would you like to take a look at this?


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2023-01-03 Thread Chuanqi Xu via Phabricator via cfe-commits

ChuanqiXu updated this revision to Diff 486132.
ChuanqiXu added a comment.

Address comments.


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

https://reviews.llvm.org/D137058

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/lit.local.cfg
  clang/test/Driver/module-output.cppm

Index: clang/test/Driver/module-output.cppm
===
--- /dev/null
+++ clang/test/Driver/module-output.cppm
@@ -0,0 +1,33 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// Tests that the .pcm file will be generated in the same directory with the specified
+// output and the name of the .pcm file should be the same with the input file.
+// RUN: %clang -std=c++20 %t/Hello.cppm -fmodule-output -c -o %t/output/Hello.o \
+// RUN:   -### 2>&1 | FileCheck %t/Hello.cppm
+//
+// Tests that the output file will be generated in the output directory when we
+// specified multiple input files.
+// RUN: %clang -std=c++20 %t/Hello.cppm %t/AnotherModule.cppm -fmodule-output -o \
+// RUN:   %t/output/a.out -### 2>&1 | FileCheck  %t/AnotherModule.cppm
+//
+// Tests that clang will reject the command line if it specifies -fmodule-output with
+// multiple archs.
+// RUN: %clang %t/Hello.cppm -fmodule-output -arch i386 -arch x86_64 -### -target \
+// RUN:   x86_64-apple-darwin 2>&1 | FileCheck %t/Hello.cppm -check-prefix=MULTIPLE-ARCH
+
+//--- Hello.cppm
+export module Hello;
+
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/output/Hello.pcm" "-x" "c++" "{{.*}}/Hello.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/output/Hello.o" "-x" "pcm" "{{.*}}/output/Hello.pcm"
+
+// MULTIPLE-ARCH: option '-fmodule-output' can't be used with multiple arch options
+
+//--- AnotherModule.cppm
+export module AnotherModule;
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/output/Hello.pcm" "-x" "c++" "{{.*}}/Hello.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/Hello-{{.*}}.o" "-x" "pcm" "{{.*}}/output/Hello.pcm"
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" "AnotherModule.cppm" {{.*}}"-o" "{{.*}}/output/AnotherModule.pcm" "-x" "c++" "{{.*}}/AnotherModule.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "AnotherModule.cppm" {{.*}}"-o" "{{.*}}/AnotherModule-{{.*}}.o" "-x" "pcm" "{{.*}}/output/AnotherModule.pcm"
Index: clang/test/Driver/lit.local.cfg
===
--- clang/test/Driver/lit.local.cfg
+++ clang/test/Driver/lit.local.cfg
@@ -1,6 +1,6 @@
 from lit.llvm import llvm_config
 
-config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',
+config.suffixes = ['.c', '.cpp', '.cppm', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',
'.cu', '.rs', '.cl', '.clcpp', '.hip', '.hlsl']
 config.substitutions = list(config.substitutions)
 config.substitutions.insert(0,
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -,9 +,19 @@
 );
   }
 
+  if (MultipleArchs && C.getArgs().hasArg(options::OPT_fmodule_output))
+Diag(clang::diag::err_drv_module_output_with_multiple_arch);
+
   // Output to a temporary file?
-  if ((!AtTopLevel && !isSaveTempsEnabled() &&
-   !C.getArgs().hasArg(options::OPT__SLASH_Fo)) ||
+  if ((!AtTopLevel &&
+   !isSaveTempsEnabled() &&
+ !C.getArgs().hasArg(options::OPT__SLASH_Fo) &&
+   // We don't generate module file to temporary file if
+   // we specified `-fmodule-output`
+   (!isa(JA) ||
+JA.getType() != types::TY_ModuleFile ||
+!C.getArgs().hasArg(options::OPT_fmodule_output) ||
+MultipleArchs)) ||
   CCGenDiagnostics) {
 StringRef Name = llvm::sys::path::filename(BaseInput);
 std::pair Split = Name.split('.');
@@ -5714,9 +5724,27 @@
 else
   llvm::sys::path::append(BasePath, NamedOutput);
 return C.addResultFile(C.getArgs().MakeArgString(BasePath.c_str()), );
-  } else {
-return C.addResultFile(NamedOutput, );
   }
+
+  // When we specified `-fmodule-output` and `-o`, the module file
+  // will be generated into the same directory with the output file
+  // and the name of the module file would be the input file with the
+  // new suffix '.pcm'.
+  if (!AtTopLevel && isa(JA) &&
+  JA.getType() == types::TY_ModuleFile &&
+  C.getArgs().hasArg(options::OPT_fmodule_output) &&
+  C.getArgs().hasArg(options::OPT_o)) {
+SmallString<128> OutputPath;
+OutputPath = C.getArgs().getLastArg(options::OPT_o)->getValue();
+llvm::sys::path::remove_filename(OutputPath);
+if (OutputPath.empty())
+  OutputPath = NamedOutput;
+

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2023-01-03 Thread Tom Honermann via Phabricator via cfe-commits

tahonermann added a comment.

I don't know the driver code very well, but as best I can tell, this appears to 
match the design agreed to at the last Clang Modules WG meeting.




Comment at: clang/test/Driver/module-output.cppm:5
+//
+// Tests that the .pcm file will be generated in the same direcotry with the 
specified
+// output and the name of the .pcm file should be the same with the input file.





Comment at: clang/test/Driver/module-output.cppm:18
+// RUN: %clang %t/Hello.cppm -fmodule-output -arch i386 -arch x86_64 -### 
-target \
+// RUN:   x86_64-apple-darwin 2>&1 | FileCheck %t/Hello.cppm 
-check-prefix=MULTIPLR-ARCH
+





Comment at: clang/test/Driver/module-output.cppm:26
+
+// MULTIPLR-ARCH: option '-fmodule-output' can't be used with multiple arch 
options
+




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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2022-12-27 Thread Chuanqi Xu via Phabricator via cfe-commits

ChuanqiXu updated this revision to Diff 485457.
ChuanqiXu added a comment.

Address comments: Reject the command line if it specifies -fmodule-output and 
multiple arch.


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

https://reviews.llvm.org/D137058

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/lit.local.cfg
  clang/test/Driver/module-output.cppm

Index: clang/test/Driver/module-output.cppm
===
--- /dev/null
+++ clang/test/Driver/module-output.cppm
@@ -0,0 +1,33 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// Tests that the .pcm file will be generated in the same direcotry with the specified
+// output and the name of the .pcm file should be the same with the input file.
+// RUN: %clang -std=c++20 %t/Hello.cppm -fmodule-output -c -o %t/output/Hello.o \
+// RUN:   -### 2>&1 | FileCheck %t/Hello.cppm
+//
+// Tests that the output file will be generated in the output directory when we
+// specified multiple input files.
+// RUN: %clang -std=c++20 %t/Hello.cppm %t/AnotherModule.cppm -fmodule-output -o \
+// RUN:   %t/output/a.out -### 2>&1 | FileCheck  %t/AnotherModule.cppm
+//
+// Tests that clang will reject the command line if it specifies -fmodule-output with
+// multiple archs.
+// RUN: %clang %t/Hello.cppm -fmodule-output -arch i386 -arch x86_64 -### -target \
+// RUN:   x86_64-apple-darwin 2>&1 | FileCheck %t/Hello.cppm -check-prefix=MULTIPLR-ARCH
+
+//--- Hello.cppm
+export module Hello;
+
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/output/Hello.pcm" "-x" "c++" "{{.*}}/Hello.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/output/Hello.o" "-x" "pcm" "{{.*}}/output/Hello.pcm"
+
+// MULTIPLR-ARCH: option '-fmodule-output' can't be used with multiple arch options
+
+//--- AnotherModule.cppm
+export module AnotherModule;
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/output/Hello.pcm" "-x" "c++" "{{.*}}/Hello.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/Hello-{{.*}}.o" "-x" "pcm" "{{.*}}/output/Hello.pcm"
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" "AnotherModule.cppm" {{.*}}"-o" "{{.*}}/output/AnotherModule.pcm" "-x" "c++" "{{.*}}/AnotherModule.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "AnotherModule.cppm" {{.*}}"-o" "{{.*}}/AnotherModule-{{.*}}.o" "-x" "pcm" "{{.*}}/output/AnotherModule.pcm"
Index: clang/test/Driver/lit.local.cfg
===
--- clang/test/Driver/lit.local.cfg
+++ clang/test/Driver/lit.local.cfg
@@ -1,6 +1,6 @@
 from lit.llvm import llvm_config
 
-config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',
+config.suffixes = ['.c', '.cpp', '.cppm', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',
'.cu', '.rs', '.cl', '.clcpp', '.hip', '.hlsl']
 config.substitutions = list(config.substitutions)
 config.substitutions.insert(0,
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -,9 +,19 @@
 );
   }
 
+  if (MultipleArchs && C.getArgs().hasArg(options::OPT_fmodule_output))
+Diag(clang::diag::err_drv_module_output_with_multiple_arch);
+
   // Output to a temporary file?
-  if ((!AtTopLevel && !isSaveTempsEnabled() &&
-   !C.getArgs().hasArg(options::OPT__SLASH_Fo)) ||
+  if ((!AtTopLevel &&
+   !isSaveTempsEnabled() &&
+ !C.getArgs().hasArg(options::OPT__SLASH_Fo) &&
+   // We don't generate module file to temporary file if
+   // we specified `-fmodule-output`
+   (!isa(JA) ||
+JA.getType() != types::TY_ModuleFile ||
+!C.getArgs().hasArg(options::OPT_fmodule_output) ||
+MultipleArchs)) ||
   CCGenDiagnostics) {
 StringRef Name = llvm::sys::path::filename(BaseInput);
 std::pair Split = Name.split('.');
@@ -5714,9 +5724,27 @@
 else
   llvm::sys::path::append(BasePath, NamedOutput);
 return C.addResultFile(C.getArgs().MakeArgString(BasePath.c_str()), );
-  } else {
-return C.addResultFile(NamedOutput, );
   }
+
+  // When we specified `-fmodule-output` and `-o`, the module file
+  // will be generated into the same directory with the output file
+  // and the name of the module file would be the input file with the
+  // new suffix '.pcm'.
+  if (!AtTopLevel && isa(JA) &&
+  JA.getType() == types::TY_ModuleFile &&
+  C.getArgs().hasArg(options::OPT_fmodule_output) &&
+  C.getArgs().hasArg(options::OPT_o)) {
+SmallString<128> OutputPath;
+OutputPath = C.getArgs().getLastArg(options::OPT_o)->getValue();
+

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2022-12-27 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

(pulling this out from an inline comment, because it's hard to quote/smaller 
than it needs to be for more complex discussions):

> Oh, thanks for finding this. It is pretty bad that I didn't image we can 
> specify multiple input module units in one command line.
>
>> What if you specify multiple source files on the command line without -c? 
>> Without -o you get test1.pcm and test2.pcm, but with -o foo you get foo.pcm 
>> overwriting foo.pcm. Perhaps if the output file specified isn't a .o file, 
>> we should ignore the -o and use the input-filename based naming? I guess we 
>> could reject this situation outright, and require the user to run multiple 
>> separate compilations. Though keeping features composable is nice.
>
> Now the name of the module file will be the same with the input file no 
> matter if we specified -o or not. With -o specified, the module files will be 
> generated into the same directory with the output file. Without -o specified, 
> the module files will be generated in the working directory. It'll still be 
> problematic if the user specify two inputs with the same name in two 
> different directory, the module file of the latter will overwrite the former 
> one. But I guess we don't need to handle such cases?
>
>> For instance - how does this interact with Apple's multiarch support (eg: 
>> clang++ test.cppm -fmodule-output -arch i386 -arch x86_64 -target 
>> x86_64_apple_darwin) - from my testing, without specifying an output file, 
>> you get something semi-usable/not simply incorrect: test-i386.pcm and 
>> test-x86_64.pcm. But if you try to name the output file you get foo.pcm and 
>> then another foo.pcm that overwrites the previous one. I think this could be 
>> taken care of if the suffix handling code was delegated down towards the 
>> else block that starts with SmallString<128> Output(getDefaultImageName());
>
> In the new implementation, I image we'll generate test-i386.pcm and 
> test-x86_64.pcm even if we specified -o with -fmodule-output. But the weird 
> thing is that when I tried to reproduce your example, the compiler told me 
> the other archs are ignored. I'm not sure if I set something wrong or I must 
> do it in Apple machine.
>
> BTW, I am not sure if I misunderstand you, but the else block that starts 
> with SmallString<128> Output(getDefaultImageName()); handles about IMAGE 
> types, which looks irreverent to me.



>> how does this interact with Apple's multiarch support
>
> Good question. What does split dwarf do in this case? Are differently named 
> outputs generated or is a multi-arch dwarf file produced (assuming they 
> exist)?

Split DWARF isn't supported on Darwin/MacOS/whatever it's called - the MachO 
debug info distribution model looks more like, but isn't quite, Split DWARF 
already (it keeps the debug info in .o files, doesn't link them into the final 
executable - and you can make a separate debug info archive with `dsymutil`).

> Rejecting the command line if it specifies multiple -arch options with 
> -fmodule-output= seems fair to me (unless/until support for multi-arch .pcm 
> files is added).

Yeah, trying to see if

>> I'm not sure if I set something wrong or I must do it in Apple machine.
>
> I don't recall for sure, but I think Apple Clang is needed. We noticed 
> differences between community Clang and Apple Clang while I was at Coverity, 
> but I don't recall the details.

I don't think this needs Apple Clang, but it does need a Darwin triple. I was 
reproducing/examining the behavior with `clang++ test.cppm -fmodule-output 
-arch i386 -arch x86_64 -### -target x86_64-apple-darwin`.


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2022-12-18 Thread Chuanqi Xu via Phabricator via cfe-commits

ChuanqiXu added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:5541-5545
+if (Arg *FinalOutput = C.getArgs().getLastArg(options::OPT_o))
+  TempPath = FinalOutput->getValue();
+else
+  TempPath = BaseInput;
+

tahonermann wrote:
> ChuanqiXu wrote:
> > dblaikie wrote:
> > > ChuanqiXu wrote:
> > > > dblaikie wrote:
> > > > > dblaikie wrote:
> > > > > > ChuanqiXu wrote:
> > > > > > > dblaikie wrote:
> > > > > > > > It'd be nice if we didn't have to recompute this/lookup `OPT_o` 
> > > > > > > > here - any way we can use the object file output path here 
> > > > > > > > (that's already handled `OPT_o` or using the base input name, 
> > > > > > > > etc)?
> > > > > > > I didn't understand this a lot. We don't compute anything here 
> > > > > > > and we just use the object file output path here if `-o` is 
> > > > > > > provided and we replace the suffix then. I feel this is simple 
> > > > > > > enough.
> > > > > > Computing the path to write to is what I'm referring to - and the 
> > > > > > fact that this had a bug (was relative to the source path instead 
> > > > > > of the CWD)/divergence from the `-o` path logic is the sort of 
> > > > > > thing I want to avoid.
> > > > > > 
> > > > > > I'm not sure there's an easy way to do this - but looks like the 
> > > > > > logic to name the .o file output is in `Driver::GetNamedOutputPath` 
> > > > > > and gets stored in the `clang::driver::Compilation` which I 
> > > > > > guess is where we are, but we're going through here with a 
> > > > > > `PrecompileJobAction` insntead of the compile job action, I suppose.
> > > > > > 
> > > > > > Could we keep these two codepaths closer together?
> > > > > > 
> > > > > > It'd be really nice if we could reuse that in some way.
> > > > > > 
> > > > > > Hmm, actually, why doesn't this fall out of the existing algorithm 
> > > > > > without modification?
> > > > > > 
> > > > > > Ah, I see, since the precompile action isn't "at top level" it gets 
> > > > > > a temporary file name - so if we change only that, it seems to fall 
> > > > > > out naturally:
> > > > > > ```
> > > > > > diff --git a/clang/lib/Driver/Driver.cpp 
> > > > > > b/clang/lib/Driver/Driver.cpp
> > > > > > index c7efe60b2335..db878cbfff46 100644
> > > > > > --- a/clang/lib/Driver/Driver.cpp
> > > > > > +++ b/clang/lib/Driver/Driver.cpp
> > > > > > @@ -5556,9 +5556,9 @@ const char 
> > > > > > *Driver::GetNamedOutputPath(Compilation , const JobAction ,
> > > > > >}
> > > > > >  
> > > > > >// Output to a temporary file?
> > > > > > -  if ((!AtTopLevel && !isSaveTempsEnabled() &&
> > > > > > +  if (((!AtTopLevel && !isSaveTempsEnabled() &&
> > > > > > !C.getArgs().hasArg(options::OPT__SLASH_Fo)) ||
> > > > > > -  CCGenDiagnostics) {
> > > > > > +  CCGenDiagnostics) && JA.getType() != types::TY_ModuleFile) {
> > > > > >  StringRef Name = llvm::sys::path::filename(BaseInput);
> > > > > >  std::pair Split = Name.split('.');
> > > > > >  const char *Suffix = types::getTypeTempSuffix(JA.getType(), 
> > > > > > IsCLMode());
> > > > > > ```
> > > > > > 
> > > > > > Without the need to reimplement/duplicate the `-o` handling logic?
> > > > > Oh, I should say, this patch didn't actually have the flag support, 
> > > > > but it'd be something like this ^ but with the command line argument 
> > > > > test as well (so "other stuff that's already there && !(TY_ModuleFile 
> > > > > && hasArg fmodule-output)")
> > > > To be honest, I prefer the previous patch. I feel it has higher 
> > > > readability. But this is a problem about taste and it doesn't have 
> > > > standard answer. Someone's readability is redundancy for others : )
> > > I think there's real functionality we're at risk of missing by having 
> > > separate implementations.
> > > 
> > > For instance - how does this interact with Apple's multiarch support (eg: 
> > > `clang++ test.cppm -fmodule-output -arch i386 -arch x86_64 -target 
> > > x86_64_apple_darwin`) - from my testing, without specifying an output 
> > > file, you get something semi-usable/not simply incorrect: `test-i386.pcm` 
> > > and `test-x86_64.pcm`. But if you try to name the output file you get 
> > > `foo.pcm` and then another `foo.pcm` that overwrites the previous one. I 
> > > think this could be taken care of if the suffix handling code was 
> > > delegated down towards the else block that starts with `SmallString<128> 
> > > Output(getDefaultImageName());`
> > > 
> > > But maybe solving that ^ problem could come out of a more general 
> > > solution to the next problem:
> > > 
> > > What if you specify multiple source files on the command line without 
> > > `-c`? Without `-o` you get `test1.pcm` and `test2.pcm`, but with `-o foo` 
> > > you get `foo.pcm` overwriting `foo.pcm`. Perhaps if the output file 
> > > specified isn't a .o file, we should ignore the `-o` and use the 
> > > input-filename based naming? I guess we could reject this

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2022-12-16 Thread Tom Honermann via Phabricator via cfe-commits

tahonermann added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:5541-5545
+if (Arg *FinalOutput = C.getArgs().getLastArg(options::OPT_o))
+  TempPath = FinalOutput->getValue();
+else
+  TempPath = BaseInput;
+

ChuanqiXu wrote:
> dblaikie wrote:
> > ChuanqiXu wrote:
> > > dblaikie wrote:
> > > > dblaikie wrote:
> > > > > ChuanqiXu wrote:
> > > > > > dblaikie wrote:
> > > > > > > It'd be nice if we didn't have to recompute this/lookup `OPT_o` 
> > > > > > > here - any way we can use the object file output path here 
> > > > > > > (that's already handled `OPT_o` or using the base input name, 
> > > > > > > etc)?
> > > > > > I didn't understand this a lot. We don't compute anything here and 
> > > > > > we just use the object file output path here if `-o` is provided 
> > > > > > and we replace the suffix then. I feel this is simple enough.
> > > > > Computing the path to write to is what I'm referring to - and the 
> > > > > fact that this had a bug (was relative to the source path instead of 
> > > > > the CWD)/divergence from the `-o` path logic is the sort of thing I 
> > > > > want to avoid.
> > > > > 
> > > > > I'm not sure there's an easy way to do this - but looks like the 
> > > > > logic to name the .o file output is in `Driver::GetNamedOutputPath` 
> > > > > and gets stored in the `clang::driver::Compilation` which I guess 
> > > > > is where we are, but we're going through here with a 
> > > > > `PrecompileJobAction` insntead of the compile job action, I suppose.
> > > > > 
> > > > > Could we keep these two codepaths closer together?
> > > > > 
> > > > > It'd be really nice if we could reuse that in some way.
> > > > > 
> > > > > Hmm, actually, why doesn't this fall out of the existing algorithm 
> > > > > without modification?
> > > > > 
> > > > > Ah, I see, since the precompile action isn't "at top level" it gets a 
> > > > > temporary file name - so if we change only that, it seems to fall out 
> > > > > naturally:
> > > > > ```
> > > > > diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
> > > > > index c7efe60b2335..db878cbfff46 100644
> > > > > --- a/clang/lib/Driver/Driver.cpp
> > > > > +++ b/clang/lib/Driver/Driver.cpp
> > > > > @@ -5556,9 +5556,9 @@ const char 
> > > > > *Driver::GetNamedOutputPath(Compilation , const JobAction ,
> > > > >}
> > > > >  
> > > > >// Output to a temporary file?
> > > > > -  if ((!AtTopLevel && !isSaveTempsEnabled() &&
> > > > > +  if (((!AtTopLevel && !isSaveTempsEnabled() &&
> > > > > !C.getArgs().hasArg(options::OPT__SLASH_Fo)) ||
> > > > > -  CCGenDiagnostics) {
> > > > > +  CCGenDiagnostics) && JA.getType() != types::TY_ModuleFile) {
> > > > >  StringRef Name = llvm::sys::path::filename(BaseInput);
> > > > >  std::pair Split = Name.split('.');
> > > > >  const char *Suffix = types::getTypeTempSuffix(JA.getType(), 
> > > > > IsCLMode());
> > > > > ```
> > > > > 
> > > > > Without the need to reimplement/duplicate the `-o` handling logic?
> > > > Oh, I should say, this patch didn't actually have the flag support, but 
> > > > it'd be something like this ^ but with the command line argument test 
> > > > as well (so "other stuff that's already there && !(TY_ModuleFile && 
> > > > hasArg fmodule-output)")
> > > To be honest, I prefer the previous patch. I feel it has higher 
> > > readability. But this is a problem about taste and it doesn't have 
> > > standard answer. Someone's readability is redundancy for others : )
> > I think there's real functionality we're at risk of missing by having 
> > separate implementations.
> > 
> > For instance - how does this interact with Apple's multiarch support (eg: 
> > `clang++ test.cppm -fmodule-output -arch i386 -arch x86_64 -target 
> > x86_64_apple_darwin`) - from my testing, without specifying an output file, 
> > you get something semi-usable/not simply incorrect: `test-i386.pcm` and 
> > `test-x86_64.pcm`. But if you try to name the output file you get `foo.pcm` 
> > and then another `foo.pcm` that overwrites the previous one. I think this 
> > could be taken care of if the suffix handling code was delegated down 
> > towards the else block that starts with `SmallString<128> 
> > Output(getDefaultImageName());`
> > 
> > But maybe solving that ^ problem could come out of a more general solution 
> > to the next problem:
> > 
> > What if you specify multiple source files on the command line without `-c`? 
> > Without `-o` you get `test1.pcm` and `test2.pcm`, but with `-o foo` you get 
> > `foo.pcm` overwriting `foo.pcm`. Perhaps if the output file specified isn't 
> > a .o file, we should ignore the `-o` and use the input-filename based 
> > naming? I guess we could reject this situation outright, and require the 
> > user to run multiple separate compilations. Though keeping features 
> > composable is nice.
> > 
> > Perhaps this needs a bit more consideration of some of these cases?
> > 
>

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2022-12-16 Thread Chuanqi Xu via Phabricator via cfe-commits

ChuanqiXu added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:5541-5545
+if (Arg *FinalOutput = C.getArgs().getLastArg(options::OPT_o))
+  TempPath = FinalOutput->getValue();
+else
+  TempPath = BaseInput;
+

dblaikie wrote:
> ChuanqiXu wrote:
> > dblaikie wrote:
> > > dblaikie wrote:
> > > > ChuanqiXu wrote:
> > > > > dblaikie wrote:
> > > > > > It'd be nice if we didn't have to recompute this/lookup `OPT_o` 
> > > > > > here - any way we can use the object file output path here (that's 
> > > > > > already handled `OPT_o` or using the base input name, etc)?
> > > > > I didn't understand this a lot. We don't compute anything here and we 
> > > > > just use the object file output path here if `-o` is provided and we 
> > > > > replace the suffix then. I feel this is simple enough.
> > > > Computing the path to write to is what I'm referring to - and the fact 
> > > > that this had a bug (was relative to the source path instead of the 
> > > > CWD)/divergence from the `-o` path logic is the sort of thing I want to 
> > > > avoid.
> > > > 
> > > > I'm not sure there's an easy way to do this - but looks like the logic 
> > > > to name the .o file output is in `Driver::GetNamedOutputPath` and gets 
> > > > stored in the `clang::driver::Compilation` which I guess is where 
> > > > we are, but we're going through here with a `PrecompileJobAction` 
> > > > insntead of the compile job action, I suppose.
> > > > 
> > > > Could we keep these two codepaths closer together?
> > > > 
> > > > It'd be really nice if we could reuse that in some way.
> > > > 
> > > > Hmm, actually, why doesn't this fall out of the existing algorithm 
> > > > without modification?
> > > > 
> > > > Ah, I see, since the precompile action isn't "at top level" it gets a 
> > > > temporary file name - so if we change only that, it seems to fall out 
> > > > naturally:
> > > > ```
> > > > diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
> > > > index c7efe60b2335..db878cbfff46 100644
> > > > --- a/clang/lib/Driver/Driver.cpp
> > > > +++ b/clang/lib/Driver/Driver.cpp
> > > > @@ -5556,9 +5556,9 @@ const char 
> > > > *Driver::GetNamedOutputPath(Compilation , const JobAction ,
> > > >}
> > > >  
> > > >// Output to a temporary file?
> > > > -  if ((!AtTopLevel && !isSaveTempsEnabled() &&
> > > > +  if (((!AtTopLevel && !isSaveTempsEnabled() &&
> > > > !C.getArgs().hasArg(options::OPT__SLASH_Fo)) ||
> > > > -  CCGenDiagnostics) {
> > > > +  CCGenDiagnostics) && JA.getType() != types::TY_ModuleFile) {
> > > >  StringRef Name = llvm::sys::path::filename(BaseInput);
> > > >  std::pair Split = Name.split('.');
> > > >  const char *Suffix = types::getTypeTempSuffix(JA.getType(), 
> > > > IsCLMode());
> > > > ```
> > > > 
> > > > Without the need to reimplement/duplicate the `-o` handling logic?
> > > Oh, I should say, this patch didn't actually have the flag support, but 
> > > it'd be something like this ^ but with the command line argument test as 
> > > well (so "other stuff that's already there && !(TY_ModuleFile && hasArg 
> > > fmodule-output)")
> > To be honest, I prefer the previous patch. I feel it has higher 
> > readability. But this is a problem about taste and it doesn't have standard 
> > answer. Someone's readability is redundancy for others : )
> I think there's real functionality we're at risk of missing by having 
> separate implementations.
> 
> For instance - how does this interact with Apple's multiarch support (eg: 
> `clang++ test.cppm -fmodule-output -arch i386 -arch x86_64 -target 
> x86_64_apple_darwin`) - from my testing, without specifying an output file, 
> you get something semi-usable/not simply incorrect: `test-i386.pcm` and 
> `test-x86_64.pcm`. But if you try to name the output file you get `foo.pcm` 
> and then another `foo.pcm` that overwrites the previous one. I think this 
> could be taken care of if the suffix handling code was delegated down towards 
> the else block that starts with `SmallString<128> 
> Output(getDefaultImageName());`
> 
> But maybe solving that ^ problem could come out of a more general solution to 
> the next problem:
> 
> What if you specify multiple source files on the command line without `-c`? 
> Without `-o` you get `test1.pcm` and `test2.pcm`, but with `-o foo` you get 
> `foo.pcm` overwriting `foo.pcm`. Perhaps if the output file specified isn't a 
> .o file, we should ignore the `-o` and use the input-filename based naming? I 
> guess we could reject this situation outright, and require the user to run 
> multiple separate compilations. Though keeping features composable is nice.
> 
> Perhaps this needs a bit more consideration of some of these cases?
> 
> 
Oh, thanks for finding this. It is pretty bad that I didn't image we can 
specify multiple input module units in one command line.

> What if you specify multiple source files on the command line without -c? 
>

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2022-12-16 Thread Chuanqi Xu via Phabricator via cfe-commits

ChuanqiXu updated this revision to Diff 483443.
ChuanqiXu added a comment.

- when `-fmodule-output` and `-o` are both specified, generate the module file 
into the directory of the output file and name the module file with the name of 
the input file with module file's suffixes. (Previously, the name of the module 
will be the same with the output file). The change tries to avoid rewrite the 
module file when we specify multiple input files in one command line.


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

https://reviews.llvm.org/D137058

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/lit.local.cfg
  clang/test/Driver/module-output.cppm

Index: clang/test/Driver/module-output.cppm
===
--- /dev/null
+++ clang/test/Driver/module-output.cppm
@@ -0,0 +1,26 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// Tests that the .pcm file will be generated in the same direcotry with the specified
+// output and the name of the .pcm file should be the same with the input file.
+// RUN: %clang -std=c++20 %t/Hello.cppm -fmodule-output -c -o %t/output/Hello.o \
+// RUN:   -### 2>&1 | FileCheck %t/Hello.cppm
+//
+// Tests that the output file will be generated in the output directory when we
+// specified multiple input files.
+// RUN: %clang -std=c++20 %t/Hello.cppm %t/AnotherModule.cppm -fmodule-output -o \
+// RUN:   %t/output/a.out -### 2>&1 | FileCheck  %t/AnotherModule.cppm
+
+//--- Hello.cppm
+export module Hello;
+
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/output/Hello.pcm" "-x" "c++" "{{.*}}/Hello.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/output/Hello.o" "-x" "pcm" "{{.*}}/output/Hello.pcm"
+
+//--- AnotherModule.cppm
+export module AnotherModule;
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/output/Hello.pcm" "-x" "c++" "{{.*}}/Hello.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/Hello-{{.*}}.o" "-x" "pcm" "{{.*}}/output/Hello.pcm"
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" "AnotherModule.cppm" {{.*}}"-o" "{{.*}}/output/AnotherModule.pcm" "-x" "c++" "{{.*}}/AnotherModule.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "AnotherModule.cppm" {{.*}}"-o" "{{.*}}/AnotherModule-{{.*}}.o" "-x" "pcm" "{{.*}}/output/AnotherModule.pcm"
Index: clang/test/Driver/lit.local.cfg
===
--- clang/test/Driver/lit.local.cfg
+++ clang/test/Driver/lit.local.cfg
@@ -1,6 +1,6 @@
 from lit.llvm import llvm_config
 
-config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',
+config.suffixes = ['.c', '.cpp', '.cppm', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',
'.cu', '.rs', '.cl', '.clcpp', '.hip', '.hlsl']
 config.substitutions = list(config.substitutions)
 config.substitutions.insert(0,
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -5556,8 +5556,14 @@
   }
 
   // Output to a temporary file?
-  if ((!AtTopLevel && !isSaveTempsEnabled() &&
-   !C.getArgs().hasArg(options::OPT__SLASH_Fo)) ||
+  if ((!AtTopLevel &&
+   !isSaveTempsEnabled() &&
+ !C.getArgs().hasArg(options::OPT__SLASH_Fo) &&
+   // We don't generate module file to temporary file if
+   // we specified `-fmodule-output`
+   (!isa(JA) ||
+JA.getType() != types::TY_ModuleFile ||
+!C.getArgs().hasArg(options::OPT_fmodule_output))) ||
   CCGenDiagnostics) {
 StringRef Name = llvm::sys::path::filename(BaseInput);
 std::pair Split = Name.split('.');
@@ -5714,9 +5720,27 @@
 else
   llvm::sys::path::append(BasePath, NamedOutput);
 return C.addResultFile(C.getArgs().MakeArgString(BasePath.c_str()), );
-  } else {
-return C.addResultFile(NamedOutput, );
   }
+
+  // When we specified `-fmodule-output` and `-o`, the module file
+  // will be generated into the same directory with the output file
+  // and the name of the module file would be the input file with the
+  // new suffix '.pcm'.
+  if (!AtTopLevel && isa(JA) &&
+  JA.getType() == types::TY_ModuleFile &&
+  C.getArgs().hasArg(options::OPT_fmodule_output) &&
+  C.getArgs().hasArg(options::OPT_o)) {
+SmallString<128> OutputPath;
+OutputPath = C.getArgs().getLastArg(options::OPT_o)->getValue();
+llvm::sys::path::remove_filename(OutputPath);
+if (OutputPath.empty())
+  OutputPath = NamedOutput;
+else
+  llvm::sys::path::append(OutputPath, NamedOutput);
+return C.addResultFile(C.getArgs().MakeArgString(OutputPath.c_str()), );
+  }
+  
+  return C.addResultFile(NamedOutput, );
 }
 
 std::string

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2022-12-15 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:5541-5545
+if (Arg *FinalOutput = C.getArgs().getLastArg(options::OPT_o))
+  TempPath = FinalOutput->getValue();
+else
+  TempPath = BaseInput;
+

ChuanqiXu wrote:
> dblaikie wrote:
> > dblaikie wrote:
> > > ChuanqiXu wrote:
> > > > dblaikie wrote:
> > > > > It'd be nice if we didn't have to recompute this/lookup `OPT_o` here 
> > > > > - any way we can use the object file output path here (that's already 
> > > > > handled `OPT_o` or using the base input name, etc)?
> > > > I didn't understand this a lot. We don't compute anything here and we 
> > > > just use the object file output path here if `-o` is provided and we 
> > > > replace the suffix then. I feel this is simple enough.
> > > Computing the path to write to is what I'm referring to - and the fact 
> > > that this had a bug (was relative to the source path instead of the 
> > > CWD)/divergence from the `-o` path logic is the sort of thing I want to 
> > > avoid.
> > > 
> > > I'm not sure there's an easy way to do this - but looks like the logic to 
> > > name the .o file output is in `Driver::GetNamedOutputPath` and gets 
> > > stored in the `clang::driver::Compilation` which I guess is where we 
> > > are, but we're going through here with a `PrecompileJobAction` insntead 
> > > of the compile job action, I suppose.
> > > 
> > > Could we keep these two codepaths closer together?
> > > 
> > > It'd be really nice if we could reuse that in some way.
> > > 
> > > Hmm, actually, why doesn't this fall out of the existing algorithm 
> > > without modification?
> > > 
> > > Ah, I see, since the precompile action isn't "at top level" it gets a 
> > > temporary file name - so if we change only that, it seems to fall out 
> > > naturally:
> > > ```
> > > diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
> > > index c7efe60b2335..db878cbfff46 100644
> > > --- a/clang/lib/Driver/Driver.cpp
> > > +++ b/clang/lib/Driver/Driver.cpp
> > > @@ -5556,9 +5556,9 @@ const char *Driver::GetNamedOutputPath(Compilation 
> > > , const JobAction ,
> > >}
> > >  
> > >// Output to a temporary file?
> > > -  if ((!AtTopLevel && !isSaveTempsEnabled() &&
> > > +  if (((!AtTopLevel && !isSaveTempsEnabled() &&
> > > !C.getArgs().hasArg(options::OPT__SLASH_Fo)) ||
> > > -  CCGenDiagnostics) {
> > > +  CCGenDiagnostics) && JA.getType() != types::TY_ModuleFile) {
> > >  StringRef Name = llvm::sys::path::filename(BaseInput);
> > >  std::pair Split = Name.split('.');
> > >  const char *Suffix = types::getTypeTempSuffix(JA.getType(), 
> > > IsCLMode());
> > > ```
> > > 
> > > Without the need to reimplement/duplicate the `-o` handling logic?
> > Oh, I should say, this patch didn't actually have the flag support, but 
> > it'd be something like this ^ but with the command line argument test as 
> > well (so "other stuff that's already there && !(TY_ModuleFile && hasArg 
> > fmodule-output)")
> To be honest, I prefer the previous patch. I feel it has higher readability. 
> But this is a problem about taste and it doesn't have standard answer. 
> Someone's readability is redundancy for others : )
I think there's real functionality we're at risk of missing by having separate 
implementations.

For instance - how does this interact with Apple's multiarch support (eg: 
`clang++ test.cppm -fmodule-output -arch i386 -arch x86_64 -target 
x86_64_apple_darwin`) - from my testing, without specifying an output file, you 
get something semi-usable/not simply incorrect: `test-i386.pcm` and 
`test-x86_64.pcm`. But if you try to name the output file you get `foo.pcm` and 
then another `foo.pcm` that overwrites the previous one. I think this could be 
taken care of if the suffix handling code was delegated down towards the else 
block that starts with `SmallString<128> Output(getDefaultImageName());`

But maybe solving that ^ problem could come out of a more general solution to 
the next problem:

What if you specify multiple source files on the command line without `-c`? 
Without `-o` you get `test1.pcm` and `test2.pcm`, but with `-o foo` you get 
`foo.pcm` overwriting `foo.pcm`. Perhaps if the output file specified isn't a 
.o file, we should ignore the `-o` and use the input-filename based naming? I 
guess we could reject this situation outright, and require the user to run 
multiple separate compilations. Though keeping features composable is nice.

Perhaps this needs a bit more consideration of some of these cases?




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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2022-12-14 Thread Chuanqi Xu via Phabricator via cfe-commits

ChuanqiXu added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:5541-5545
+if (Arg *FinalOutput = C.getArgs().getLastArg(options::OPT_o))
+  TempPath = FinalOutput->getValue();
+else
+  TempPath = BaseInput;
+

dblaikie wrote:
> dblaikie wrote:
> > ChuanqiXu wrote:
> > > dblaikie wrote:
> > > > It'd be nice if we didn't have to recompute this/lookup `OPT_o` here - 
> > > > any way we can use the object file output path here (that's already 
> > > > handled `OPT_o` or using the base input name, etc)?
> > > I didn't understand this a lot. We don't compute anything here and we 
> > > just use the object file output path here if `-o` is provided and we 
> > > replace the suffix then. I feel this is simple enough.
> > Computing the path to write to is what I'm referring to - and the fact that 
> > this had a bug (was relative to the source path instead of the 
> > CWD)/divergence from the `-o` path logic is the sort of thing I want to 
> > avoid.
> > 
> > I'm not sure there's an easy way to do this - but looks like the logic to 
> > name the .o file output is in `Driver::GetNamedOutputPath` and gets stored 
> > in the `clang::driver::Compilation` which I guess is where we are, but 
> > we're going through here with a `PrecompileJobAction` insntead of the 
> > compile job action, I suppose.
> > 
> > Could we keep these two codepaths closer together?
> > 
> > It'd be really nice if we could reuse that in some way.
> > 
> > Hmm, actually, why doesn't this fall out of the existing algorithm without 
> > modification?
> > 
> > Ah, I see, since the precompile action isn't "at top level" it gets a 
> > temporary file name - so if we change only that, it seems to fall out 
> > naturally:
> > ```
> > diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
> > index c7efe60b2335..db878cbfff46 100644
> > --- a/clang/lib/Driver/Driver.cpp
> > +++ b/clang/lib/Driver/Driver.cpp
> > @@ -5556,9 +5556,9 @@ const char *Driver::GetNamedOutputPath(Compilation 
> > , const JobAction ,
> >}
> >  
> >// Output to a temporary file?
> > -  if ((!AtTopLevel && !isSaveTempsEnabled() &&
> > +  if (((!AtTopLevel && !isSaveTempsEnabled() &&
> > !C.getArgs().hasArg(options::OPT__SLASH_Fo)) ||
> > -  CCGenDiagnostics) {
> > +  CCGenDiagnostics) && JA.getType() != types::TY_ModuleFile) {
> >  StringRef Name = llvm::sys::path::filename(BaseInput);
> >  std::pair Split = Name.split('.');
> >  const char *Suffix = types::getTypeTempSuffix(JA.getType(), 
> > IsCLMode());
> > ```
> > 
> > Without the need to reimplement/duplicate the `-o` handling logic?
> Oh, I should say, this patch didn't actually have the flag support, but it'd 
> be something like this ^ but with the command line argument test as well (so 
> "other stuff that's already there && !(TY_ModuleFile && hasArg 
> fmodule-output)")
To be honest, I prefer the previous patch. I feel it has higher readability. 
But this is a problem about taste and it doesn't have standard answer. 
Someone's readability is redundancy for others : )


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2022-12-14 Thread Chuanqi Xu via Phabricator via cfe-commits

ChuanqiXu updated this revision to Diff 483074.
ChuanqiXu added a comment.

Address comments:

- Reuse the logic to compute object file for module file when `-o` is not 
specified.


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

https://reviews.llvm.org/D137058

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/lit.local.cfg
  clang/test/Driver/module-output.cppm


Index: clang/test/Driver/module-output.cppm
===
--- /dev/null
+++ clang/test/Driver/module-output.cppm
@@ -0,0 +1,9 @@
+// Tests that the .pcm file will be generated in the same direcotry with the 
specified 
+// output and the name of the .pcm file should be the same with the output 
file.
+// RUN: %clang -std=c++20 %s -fmodule-output -c -o %t/output/Hello.o \
+// RUN:   -### 2>&1 | FileCheck %s
+
+export module Hello;
+
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" 
"module-output.cppm" {{.*}}"-o" "{{.*}}/Hello.pcm" "-x" "c++" 
"{{.*}}/module-output.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "module-output.cppm" {{.*}}"-o" 
"{{.*}}/Hello.o" "-x" "pcm" "{{.*}}/Hello.pcm"
Index: clang/test/Driver/lit.local.cfg
===
--- clang/test/Driver/lit.local.cfg
+++ clang/test/Driver/lit.local.cfg
@@ -1,6 +1,6 @@
 from lit.llvm import llvm_config
 
-config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', 
'.F90', '.f95',
+config.suffixes = ['.c', '.cpp', '.cppm', '.h', '.m', '.mm', '.S', '.s', 
'.f90', '.F90', '.f95',
'.cu', '.rs', '.cl', '.clcpp', '.hip', '.hlsl']
 config.substitutions = list(config.substitutions)
 config.substitutions.insert(0,
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -5540,6 +5540,23 @@
 return "-";
   }
 
+  // If `-fmodule-output` is specfied, then:
+  // - If `-o` is specified, the module file is written to the same path
+  //   with the output filename in module file's suffix. 
+  // - If `-o` is not specified, the module file is written to the working
+  //   direcotry with the input filename in module file's suffix. The logic
+  //   falls out to reuse the logic to compute the path of the object files.
+  if (!AtTopLevel && isa(JA) &&
+  JA.getType() == types::TY_ModuleFile &&
+  C.getArgs().hasArg(options::OPT_fmodule_output) &&
+  C.getArgs().hasArg(options::OPT_o)) {
+SmallString<128> TempPath;
+TempPath = C.getArgs().getLastArg(options::OPT_o)->getValue();
+const char *Extension = types::getTypeTempSuffix(JA.getType());
+llvm::sys::path::replace_extension(TempPath, Extension);
+return C.addResultFile(C.getArgs().MakeArgString(TempPath.c_str()), );
+  }
+
   if (IsDXCMode() && !C.getArgs().hasArg(options::OPT_o))
 return "-";
 
@@ -5556,8 +5573,14 @@
   }
 
   // Output to a temporary file?
-  if ((!AtTopLevel && !isSaveTempsEnabled() &&
-   !C.getArgs().hasArg(options::OPT__SLASH_Fo)) ||
+  if ((!AtTopLevel &&
+   !isSaveTempsEnabled() &&
+ !C.getArgs().hasArg(options::OPT__SLASH_Fo) &&
+   // We don't generate module file to temporary file if
+   // we specified `-fmodule-output`
+   (!isa(JA) ||
+JA.getType() != types::TY_ModuleFile ||
+!C.getArgs().hasArg(options::OPT_fmodule_output))) ||
   CCGenDiagnostics) {
 StringRef Name = llvm::sys::path::filename(BaseInput);
 std::pair Split = Name.split('.');
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2285,6 +2285,9 @@
   PosFlag,
   NegFlag, BothFlags<[NoXarchOption, CC1Option]>>;
 
+def fmodule_output : Flag<["-"], "fmodule-output">, Flags<[NoXarchOption, 
CC1Option]>,
+  HelpText<"Save intermediate module file results when compiling a standard 
C++ module unit.">;
+
 def fmodules_prune_interval : Joined<["-"], "fmodules-prune-interval=">, 
Group,
   Flags<[CC1Option]>, MetaVarName<"">,
   HelpText<"Specify the interval (in seconds) between attempts to prune the 
module cache">,


Index: clang/test/Driver/module-output.cppm
===
--- /dev/null
+++ clang/test/Driver/module-output.cppm
@@ -0,0 +1,9 @@
+// Tests that the .pcm file will be generated in the same direcotry with the specified 
+// output and the name of the .pcm file should be the same with the output file.
+// RUN: %clang -std=c++20 %s -fmodule-output -c -o %t/output/Hello.o \
+// RUN:   -### 2>&1 | FileCheck %s
+
+export module Hello;
+
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" "module-output.cppm" {{.*}}"-o" "{{.*}}/Hello.pcm" "-x" "c++" "{{.*}}/module-output.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name"

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2022-12-14 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:5541-5545
+if (Arg *FinalOutput = C.getArgs().getLastArg(options::OPT_o))
+  TempPath = FinalOutput->getValue();
+else
+  TempPath = BaseInput;
+

dblaikie wrote:
> ChuanqiXu wrote:
> > dblaikie wrote:
> > > It'd be nice if we didn't have to recompute this/lookup `OPT_o` here - 
> > > any way we can use the object file output path here (that's already 
> > > handled `OPT_o` or using the base input name, etc)?
> > I didn't understand this a lot. We don't compute anything here and we just 
> > use the object file output path here if `-o` is provided and we replace the 
> > suffix then. I feel this is simple enough.
> Computing the path to write to is what I'm referring to - and the fact that 
> this had a bug (was relative to the source path instead of the 
> CWD)/divergence from the `-o` path logic is the sort of thing I want to avoid.
> 
> I'm not sure there's an easy way to do this - but looks like the logic to 
> name the .o file output is in `Driver::GetNamedOutputPath` and gets stored in 
> the `clang::driver::Compilation` which I guess is where we are, but we're 
> going through here with a `PrecompileJobAction` insntead of the compile job 
> action, I suppose.
> 
> Could we keep these two codepaths closer together?
> 
> It'd be really nice if we could reuse that in some way.
> 
> Hmm, actually, why doesn't this fall out of the existing algorithm without 
> modification?
> 
> Ah, I see, since the precompile action isn't "at top level" it gets a 
> temporary file name - so if we change only that, it seems to fall out 
> naturally:
> ```
> diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
> index c7efe60b2335..db878cbfff46 100644
> --- a/clang/lib/Driver/Driver.cpp
> +++ b/clang/lib/Driver/Driver.cpp
> @@ -5556,9 +5556,9 @@ const char *Driver::GetNamedOutputPath(Compilation , 
> const JobAction ,
>}
>  
>// Output to a temporary file?
> -  if ((!AtTopLevel && !isSaveTempsEnabled() &&
> +  if (((!AtTopLevel && !isSaveTempsEnabled() &&
> !C.getArgs().hasArg(options::OPT__SLASH_Fo)) ||
> -  CCGenDiagnostics) {
> +  CCGenDiagnostics) && JA.getType() != types::TY_ModuleFile) {
>  StringRef Name = llvm::sys::path::filename(BaseInput);
>  std::pair Split = Name.split('.');
>  const char *Suffix = types::getTypeTempSuffix(JA.getType(), IsCLMode());
> ```
> 
> Without the need to reimplement/duplicate the `-o` handling logic?
Oh, I should say, this patch didn't actually have the flag support, but it'd be 
something like this ^ but with the command line argument test as well (so 
"other stuff that's already there && !(TY_ModuleFile && hasArg fmodule-output)")


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2022-12-14 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:5541-5545
+if (Arg *FinalOutput = C.getArgs().getLastArg(options::OPT_o))
+  TempPath = FinalOutput->getValue();
+else
+  TempPath = BaseInput;
+

ChuanqiXu wrote:
> dblaikie wrote:
> > It'd be nice if we didn't have to recompute this/lookup `OPT_o` here - any 
> > way we can use the object file output path here (that's already handled 
> > `OPT_o` or using the base input name, etc)?
> I didn't understand this a lot. We don't compute anything here and we just 
> use the object file output path here if `-o` is provided and we replace the 
> suffix then. I feel this is simple enough.
Computing the path to write to is what I'm referring to - and the fact that 
this had a bug (was relative to the source path instead of the CWD)/divergence 
from the `-o` path logic is the sort of thing I want to avoid.

I'm not sure there's an easy way to do this - but looks like the logic to name 
the .o file output is in `Driver::GetNamedOutputPath` and gets stored in the 
`clang::driver::Compilation` which I guess is where we are, but we're going 
through here with a `PrecompileJobAction` insntead of the compile job action, I 
suppose.

Could we keep these two codepaths closer together?

It'd be really nice if we could reuse that in some way.

Hmm, actually, why doesn't this fall out of the existing algorithm without 
modification?

Ah, I see, since the precompile action isn't "at top level" it gets a temporary 
file name - so if we change only that, it seems to fall out naturally:
```
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index c7efe60b2335..db878cbfff46 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -5556,9 +5556,9 @@ const char *Driver::GetNamedOutputPath(Compilation , 
const JobAction ,
   }
 
   // Output to a temporary file?
-  if ((!AtTopLevel && !isSaveTempsEnabled() &&
+  if (((!AtTopLevel && !isSaveTempsEnabled() &&
!C.getArgs().hasArg(options::OPT__SLASH_Fo)) ||
-  CCGenDiagnostics) {
+  CCGenDiagnostics) && JA.getType() != types::TY_ModuleFile) {
 StringRef Name = llvm::sys::path::filename(BaseInput);
 std::pair Split = Name.split('.');
 const char *Suffix = types::getTypeTempSuffix(JA.getType(), IsCLMode());
```

Without the need to reimplement/duplicate the `-o` handling logic?


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2022-12-14 Thread Chuanqi Xu via Phabricator via cfe-commits

ChuanqiXu added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:5541-5542
+  //
+  // FIXME: Do we need to handle the case that the calculated output is
+  // conflicting with the specified output file or the input file?
+  if (!AtTopLevel && isa(JA) &&

tahonermann wrote:
> tahonermann wrote:
> > ChuanqiXu wrote:
> > > dblaikie wrote:
> > > > tahonermann wrote:
> > > > > ChuanqiXu wrote:
> > > > > > dblaikie wrote:
> > > > > > > Do we do that for `-o` today? (eg: if you try to `-o` and specify 
> > > > > > > the input file name, such that the output would overwrite the 
> > > > > > > input, what happens? I'm not sure - but I guess it's OK to do 
> > > > > > > whatever that is for this case too)
> > > > > > > Do we do that for -o today? (eg: if you try to -o and specify the 
> > > > > > > input file name, such that the output would overwrite the input, 
> > > > > > > what happens? I'm not sure - but I guess it's OK to do whatever 
> > > > > > > that is for this case too)
> > > > > > 
> > > > > > In this case, the input file will be overwrite and no warning 
> > > > > > shows. So it looks like we didn't take special treatment here. So I 
> > > > > > remove the FIXME.
> > > > > Basing the location of the module output on the presence of `-o` 
> > > > > sounds confusing to me. Likewise, defaulting to writing next to the 
> > > > > input file as opposed to the current directory (where a `.o` file is 
> > > > > written by default) sounds wrong. I think this option should be 
> > > > > handled similarly to `-o`; it should accept a path and:
> > > > >   - If an absolute path is provided, write to that location.
> > > > >   - If a relative path is provided, write to that location relative 
> > > > > to the current working directory.
> > > > > Leave it up to the user or build system to ensure that the `.o` and 
> > > > > `.pcm` file locations coincide if that is what they want. In general, 
> > > > > I don't expect colocation of `.o` and `.pcm` files to be what is 
> > > > > desired.
> > > > > 
> > > > > 
> > > > @tahonermann there's precedent for this with Split DWARF (.dwo files go 
> > > > next to the .o file) & I'd argued for possibly only providing this 
> > > > behavior, letting consumers move files elsewhere if they needed to, but 
> > > > got voted down there.
> > > > 
> > > > I do think this is a reasonable default, though. Users and build 
> > > > systems have the option to pass a path to place the .pcm somewhere else 
> > > > (in the follow-up patch to this one).
> > > @tahonermann here is another patch which implements the behavior you 
> > > described: https://reviews.llvm.org/D137059
> > I'm ok with a default that consistently writes the PCM relative to the 
> > location of the `.o` file (if not overridden with an absolute path). What 
> > I'm not ok with is having the default be next to the `.o` file if `-o` is 
> > specified and next to the input file if `-o` is not specified. I don't 
> > think writing the PCM relative to the input file is a good default in any 
> > case. If `-o` is not specified, then I think it should be written relative 
> > to the current working directory; which matches where the `.o` file will be 
> > written.
> Summary of the consensus of the Clang Modules WG:
>   # If `-fmodule-output=` is provided, the PCM is written to the indicated 
> file (relative to the current working directory for a relative path).
>   # If `-fmodule-output` is provided, the PCM is written relative to the 
> location of the `.o` file (the current working directory by default and the 
> location indicated by `-o` otherwise).
Done.


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2022-12-14 Thread Chuanqi Xu via Phabricator via cfe-commits

ChuanqiXu updated this revision to Diff 482738.
ChuanqiXu edited the summary of this revision.
ChuanqiXu added a comment.

Address comments:

- if `-o` is not provided, emit the module file to the working directory 
instead of the directory of the input file.


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

https://reviews.llvm.org/D137058

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/lit.local.cfg
  clang/test/Driver/module-output.cppm


Index: clang/test/Driver/module-output.cppm
===
--- /dev/null
+++ clang/test/Driver/module-output.cppm
@@ -0,0 +1,9 @@
+// Tests that the .pcm file will be generated in the same direcotry with the 
specified 
+// output and the name of the .pcm file should be the same with the output 
file.
+// RUN: %clang -std=c++20 %s -fmodule-output -c -o %t/output/Hello.o \
+// RUN:   -### 2>&1 | FileCheck %s
+
+export module Hello;
+
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" 
"module-output.cppm" {{.*}}"-o" "{{.*}}/Hello.pcm" "-x" "c++" 
"{{.*}}/module-output.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "module-output.cppm" {{.*}}"-o" 
"{{.*}}/Hello.o" "-x" "pcm" "{{.*}}/Hello.pcm"
Index: clang/test/Driver/lit.local.cfg
===
--- clang/test/Driver/lit.local.cfg
+++ clang/test/Driver/lit.local.cfg
@@ -1,6 +1,6 @@
 from lit.llvm import llvm_config
 
-config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', 
'.F90', '.f95',
+config.suffixes = ['.c', '.cpp', '.cppm', '.h', '.m', '.mm', '.S', '.s', 
'.f90', '.F90', '.f95',
'.cu', '.rs', '.cl', '.clcpp', '.hip', '.hlsl']
 config.substitutions = list(config.substitutions)
 config.substitutions.insert(0,
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -5540,6 +5540,27 @@
 return "-";
   }
 
+  // If `-fmodule-output` is specfied, then:
+  // - If `-o` is specified, the module file is written to the same path
+  //   with the output filename in module file's suffix. 
+  // - If `-o` is not specified, the module file is written to the working
+  //   direcotry with the input filename in module file's suffix.
+  if (!AtTopLevel && isa(JA) &&
+  JA.getType() == types::TY_ModuleFile &&
+  C.getArgs().hasArg(options::OPT_fmodule_output)) {
+SmallString<128> TempPath;
+if (Arg *FinalOutput = C.getArgs().getLastArg(options::OPT_o))
+  TempPath = FinalOutput->getValue();
+else {
+  llvm::sys::fs::current_path(TempPath);
+  llvm::sys::path::append(TempPath, llvm::sys::path::filename(BaseInput));
+}
+
+const char *Extension = types::getTypeTempSuffix(JA.getType());
+llvm::sys::path::replace_extension(TempPath, Extension);
+return C.addResultFile(C.getArgs().MakeArgString(TempPath.c_str()), );
+  }
+
   if (IsDXCMode() && !C.getArgs().hasArg(options::OPT_o))
 return "-";
 
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2285,6 +2285,9 @@
   PosFlag,
   NegFlag, BothFlags<[NoXarchOption, CC1Option]>>;
 
+def fmodule_output : Flag<["-"], "fmodule-output">, Flags<[NoXarchOption, 
CC1Option]>,
+  HelpText<"Save intermediate module file results when compiling a standard 
C++ module unit.">;
+
 def fmodules_prune_interval : Joined<["-"], "fmodules-prune-interval=">, 
Group,
   Flags<[CC1Option]>, MetaVarName<"">,
   HelpText<"Specify the interval (in seconds) between attempts to prune the 
module cache">,


Index: clang/test/Driver/module-output.cppm
===
--- /dev/null
+++ clang/test/Driver/module-output.cppm
@@ -0,0 +1,9 @@
+// Tests that the .pcm file will be generated in the same direcotry with the specified 
+// output and the name of the .pcm file should be the same with the output file.
+// RUN: %clang -std=c++20 %s -fmodule-output -c -o %t/output/Hello.o \
+// RUN:   -### 2>&1 | FileCheck %s
+
+export module Hello;
+
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" "module-output.cppm" {{.*}}"-o" "{{.*}}/Hello.pcm" "-x" "c++" "{{.*}}/module-output.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "module-output.cppm" {{.*}}"-o" "{{.*}}/Hello.o" "-x" "pcm" "{{.*}}/Hello.pcm"
Index: clang/test/Driver/lit.local.cfg
===
--- clang/test/Driver/lit.local.cfg
+++ clang/test/Driver/lit.local.cfg
@@ -1,6 +1,6 @@
 from lit.llvm import llvm_config
 
-config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',
+config.suffixes = ['.c', '.cpp', '.cppm', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2022-12-13 Thread Tom Honermann via Phabricator via cfe-commits

tahonermann added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:5541-5542
+  //
+  // FIXME: Do we need to handle the case that the calculated output is
+  // conflicting with the specified output file or the input file?
+  if (!AtTopLevel && isa(JA) &&

tahonermann wrote:
> ChuanqiXu wrote:
> > dblaikie wrote:
> > > tahonermann wrote:
> > > > ChuanqiXu wrote:
> > > > > dblaikie wrote:
> > > > > > Do we do that for `-o` today? (eg: if you try to `-o` and specify 
> > > > > > the input file name, such that the output would overwrite the 
> > > > > > input, what happens? I'm not sure - but I guess it's OK to do 
> > > > > > whatever that is for this case too)
> > > > > > Do we do that for -o today? (eg: if you try to -o and specify the 
> > > > > > input file name, such that the output would overwrite the input, 
> > > > > > what happens? I'm not sure - but I guess it's OK to do whatever 
> > > > > > that is for this case too)
> > > > > 
> > > > > In this case, the input file will be overwrite and no warning shows. 
> > > > > So it looks like we didn't take special treatment here. So I remove 
> > > > > the FIXME.
> > > > Basing the location of the module output on the presence of `-o` sounds 
> > > > confusing to me. Likewise, defaulting to writing next to the input file 
> > > > as opposed to the current directory (where a `.o` file is written by 
> > > > default) sounds wrong. I think this option should be handled similarly 
> > > > to `-o`; it should accept a path and:
> > > >   - If an absolute path is provided, write to that location.
> > > >   - If a relative path is provided, write to that location relative to 
> > > > the current working directory.
> > > > Leave it up to the user or build system to ensure that the `.o` and 
> > > > `.pcm` file locations coincide if that is what they want. In general, I 
> > > > don't expect colocation of `.o` and `.pcm` files to be what is desired.
> > > > 
> > > > 
> > > @tahonermann there's precedent for this with Split DWARF (.dwo files go 
> > > next to the .o file) & I'd argued for possibly only providing this 
> > > behavior, letting consumers move files elsewhere if they needed to, but 
> > > got voted down there.
> > > 
> > > I do think this is a reasonable default, though. Users and build systems 
> > > have the option to pass a path to place the .pcm somewhere else (in the 
> > > follow-up patch to this one).
> > @tahonermann here is another patch which implements the behavior you 
> > described: https://reviews.llvm.org/D137059
> I'm ok with a default that consistently writes the PCM relative to the 
> location of the `.o` file (if not overridden with an absolute path). What I'm 
> not ok with is having the default be next to the `.o` file if `-o` is 
> specified and next to the input file if `-o` is not specified. I don't think 
> writing the PCM relative to the input file is a good default in any case. If 
> `-o` is not specified, then I think it should be written relative to the 
> current working directory; which matches where the `.o` file will be written.
Summary of the consensus of the Clang Modules WG:
  # If `-fmodule-output=` is provided, the PCM is written to the indicated file 
(relative to the current working directory for a relative path).
  # If `-fmodule-output` is provided, the PCM is written relative to the 
location of the `.o` file (the current working directory by default and the 
location indicated by `-o` otherwise).


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2022-12-13 Thread Tom Honermann via Phabricator via cfe-commits

tahonermann added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:5541-5542
+  //
+  // FIXME: Do we need to handle the case that the calculated output is
+  // conflicting with the specified output file or the input file?
+  if (!AtTopLevel && isa(JA) &&

ChuanqiXu wrote:
> dblaikie wrote:
> > tahonermann wrote:
> > > ChuanqiXu wrote:
> > > > dblaikie wrote:
> > > > > Do we do that for `-o` today? (eg: if you try to `-o` and specify the 
> > > > > input file name, such that the output would overwrite the input, what 
> > > > > happens? I'm not sure - but I guess it's OK to do whatever that is 
> > > > > for this case too)
> > > > > Do we do that for -o today? (eg: if you try to -o and specify the 
> > > > > input file name, such that the output would overwrite the input, what 
> > > > > happens? I'm not sure - but I guess it's OK to do whatever that is 
> > > > > for this case too)
> > > > 
> > > > In this case, the input file will be overwrite and no warning shows. So 
> > > > it looks like we didn't take special treatment here. So I remove the 
> > > > FIXME.
> > > Basing the location of the module output on the presence of `-o` sounds 
> > > confusing to me. Likewise, defaulting to writing next to the input file 
> > > as opposed to the current directory (where a `.o` file is written by 
> > > default) sounds wrong. I think this option should be handled similarly to 
> > > `-o`; it should accept a path and:
> > >   - If an absolute path is provided, write to that location.
> > >   - If a relative path is provided, write to that location relative to 
> > > the current working directory.
> > > Leave it up to the user or build system to ensure that the `.o` and 
> > > `.pcm` file locations coincide if that is what they want. In general, I 
> > > don't expect colocation of `.o` and `.pcm` files to be what is desired.
> > > 
> > > 
> > @tahonermann there's precedent for this with Split DWARF (.dwo files go 
> > next to the .o file) & I'd argued for possibly only providing this 
> > behavior, letting consumers move files elsewhere if they needed to, but got 
> > voted down there.
> > 
> > I do think this is a reasonable default, though. Users and build systems 
> > have the option to pass a path to place the .pcm somewhere else (in the 
> > follow-up patch to this one).
> @tahonermann here is another patch which implements the behavior you 
> described: https://reviews.llvm.org/D137059
I'm ok with a default that consistently writes the PCM relative to the location 
of the `.o` file (if not overridden with an absolute path). What I'm not ok 
with is having the default be next to the `.o` file if `-o` is specified and 
next to the input file if `-o` is not specified. I don't think writing the PCM 
relative to the input file is a good default in any case. If `-o` is not 
specified, then I think it should be written relative to the current working 
directory; which matches where the `.o` file will be written.


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2022-12-12 Thread Chuanqi Xu via Phabricator via cfe-commits

ChuanqiXu added a comment.

> (I'd probably still reduce the test down to one example, using -o and 
> skipping the extra OUTPUT_PATH details, only checking the last part of the 
> path is as specified (or perhaps checking that it matches the .o file?))

Done.

> Also, could you consider the questions @urnathan asked in the 
> cross-project/flag naming thread about the semantics of this flag - I imagine 
> the right behavior is "whatever we do for .o files", but it'd be good to 
> check/consider/test them (they may not need automated testing - if the 
> behaviour is implemented with the same utilities for .o files as for .pcm 
> files then I don't mind just a statement that that's the case and some manual 
> testing has been done to verify that all works)?

Yeah, I've replied in that thread. And it looks like we can't test the 
behaviors if we can only check the output of `-###` in this patch. I've made 
some manual testing locally and let's document the behaviors later.




Comment at: clang/lib/Driver/Driver.cpp:5541-5545
+if (Arg *FinalOutput = C.getArgs().getLastArg(options::OPT_o))
+  TempPath = FinalOutput->getValue();
+else
+  TempPath = BaseInput;
+

dblaikie wrote:
> It'd be nice if we didn't have to recompute this/lookup `OPT_o` here - any 
> way we can use the object file output path here (that's already handled 
> `OPT_o` or using the base input name, etc)?
I didn't understand this a lot. We don't compute anything here and we just use 
the object file output path here if `-o` is provided and we replace the suffix 
then. I feel this is simple enough.



Comment at: clang/lib/Driver/Driver.cpp:5541-5542
+  //
+  // FIXME: Do we need to handle the case that the calculated output is
+  // conflicting with the specified output file or the input file?
+  if (!AtTopLevel && isa(JA) &&

dblaikie wrote:
> tahonermann wrote:
> > ChuanqiXu wrote:
> > > dblaikie wrote:
> > > > Do we do that for `-o` today? (eg: if you try to `-o` and specify the 
> > > > input file name, such that the output would overwrite the input, what 
> > > > happens? I'm not sure - but I guess it's OK to do whatever that is for 
> > > > this case too)
> > > > Do we do that for -o today? (eg: if you try to -o and specify the input 
> > > > file name, such that the output would overwrite the input, what 
> > > > happens? I'm not sure - but I guess it's OK to do whatever that is for 
> > > > this case too)
> > > 
> > > In this case, the input file will be overwrite and no warning shows. So 
> > > it looks like we didn't take special treatment here. So I remove the 
> > > FIXME.
> > Basing the location of the module output on the presence of `-o` sounds 
> > confusing to me. Likewise, defaulting to writing next to the input file as 
> > opposed to the current directory (where a `.o` file is written by default) 
> > sounds wrong. I think this option should be handled similarly to `-o`; it 
> > should accept a path and:
> >   - If an absolute path is provided, write to that location.
> >   - If a relative path is provided, write to that location relative to the 
> > current working directory.
> > Leave it up to the user or build system to ensure that the `.o` and `.pcm` 
> > file locations coincide if that is what they want. In general, I don't 
> > expect colocation of `.o` and `.pcm` files to be what is desired.
> > 
> > 
> @tahonermann there's precedent for this with Split DWARF (.dwo files go next 
> to the .o file) & I'd argued for possibly only providing this behavior, 
> letting consumers move files elsewhere if they needed to, but got voted down 
> there.
> 
> I do think this is a reasonable default, though. Users and build systems have 
> the option to pass a path to place the .pcm somewhere else (in the follow-up 
> patch to this one).
@tahonermann here is another patch which implements the behavior you described: 
https://reviews.llvm.org/D137059


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2022-12-12 Thread Chuanqi Xu via Phabricator via cfe-commits

ChuanqiXu updated this revision to Diff 482359.
ChuanqiXu marked an inline comment as done.
ChuanqiXu added a comment.

- Reduce the test down to one example, using -o and skipping the extra 
OUTPUT_PATH details - as the comment required.


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

https://reviews.llvm.org/D137058

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/lit.local.cfg
  clang/test/Driver/module-output.cppm


Index: clang/test/Driver/module-output.cppm
===
--- /dev/null
+++ clang/test/Driver/module-output.cppm
@@ -0,0 +1,9 @@
+// Tests that the .pcm file will be generated in the same direcotry with the 
specified 
+// output and the name of the .pcm file should be the same with the output 
file.
+// RUN: %clang -std=c++20 %s -fmodule-output -c -o %t/output/Hello.o \
+// RUN:   -### 2>&1 | FileCheck %s
+
+export module Hello;
+
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" 
"module-output.cppm" {{.*}}"-o" "{{.*}}/Hello.pcm" "-x" "c++" 
"{{.*}}/module-output.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "module-output.cppm" {{.*}}"-o" 
"{{.*}}/Hello.o" "-x" "pcm" "{{.*}}/Hello.pcm"
Index: clang/test/Driver/lit.local.cfg
===
--- clang/test/Driver/lit.local.cfg
+++ clang/test/Driver/lit.local.cfg
@@ -1,6 +1,6 @@
 from lit.llvm import llvm_config
 
-config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', 
'.F90', '.f95',
+config.suffixes = ['.c', '.cpp', '.cppm', '.h', '.m', '.mm', '.S', '.s', 
'.f90', '.F90', '.f95',
'.cu', '.rs', '.cl', '.clcpp', '.hip', '.hlsl']
 config.substitutions = list(config.substitutions)
 config.substitutions.insert(0,
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -5529,6 +5529,25 @@
 return "-";
   }
 
+  // If `-fmodule-output` is specfied, then:
+  // - If `-o` is specified, the module file is writing to the same path
+  //   with the output file in module file's suffix. 
+  // - If `-o` is not specified, the module file is writing to the same path
+  //   with the input file in module file's suffix.
+  if (!AtTopLevel && isa(JA) &&
+  JA.getType() == types::TY_ModuleFile &&
+  C.getArgs().hasArg(options::OPT_fmodule_output)) {
+SmallString<128> TempPath;
+if (Arg *FinalOutput = C.getArgs().getLastArg(options::OPT_o))
+  TempPath = FinalOutput->getValue();
+else
+  TempPath = BaseInput;
+
+const char *Extension = types::getTypeTempSuffix(JA.getType());
+llvm::sys::path::replace_extension(TempPath, Extension);
+return C.addResultFile(C.getArgs().MakeArgString(TempPath.c_str()), );
+  }
+
   if (IsDXCMode() && !C.getArgs().hasArg(options::OPT_o))
 return "-";
 
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2285,6 +2285,9 @@
   PosFlag,
   NegFlag, BothFlags<[NoXarchOption, CC1Option]>>;
 
+def fmodule_output : Flag<["-"], "fmodule-output">, Flags<[NoXarchOption, 
CC1Option]>,
+  HelpText<"Save intermediate module file results when compiling a standard 
C++ module unit.">;
+
 def fmodules_prune_interval : Joined<["-"], "fmodules-prune-interval=">, 
Group,
   Flags<[CC1Option]>, MetaVarName<"">,
   HelpText<"Specify the interval (in seconds) between attempts to prune the 
module cache">,


Index: clang/test/Driver/module-output.cppm
===
--- /dev/null
+++ clang/test/Driver/module-output.cppm
@@ -0,0 +1,9 @@
+// Tests that the .pcm file will be generated in the same direcotry with the specified 
+// output and the name of the .pcm file should be the same with the output file.
+// RUN: %clang -std=c++20 %s -fmodule-output -c -o %t/output/Hello.o \
+// RUN:   -### 2>&1 | FileCheck %s
+
+export module Hello;
+
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" "module-output.cppm" {{.*}}"-o" "{{.*}}/Hello.pcm" "-x" "c++" "{{.*}}/module-output.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "module-output.cppm" {{.*}}"-o" "{{.*}}/Hello.o" "-x" "pcm" "{{.*}}/Hello.pcm"
Index: clang/test/Driver/lit.local.cfg
===
--- clang/test/Driver/lit.local.cfg
+++ clang/test/Driver/lit.local.cfg
@@ -1,6 +1,6 @@
 from lit.llvm import llvm_config
 
-config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',
+config.suffixes = ['.c', '.cpp', '.cppm', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',
'.cu', '.rs', '.cl', '.clcpp', '.hip', '.hlsl']
 config.substitutions = list(config.substitutions)
 config.substitutions.insert(0,

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2022-12-12 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:5541-5545
+if (Arg *FinalOutput = C.getArgs().getLastArg(options::OPT_o))
+  TempPath = FinalOutput->getValue();
+else
+  TempPath = BaseInput;
+

It'd be nice if we didn't have to recompute this/lookup `OPT_o` here - any way 
we can use the object file output path here (that's already handled `OPT_o` or 
using the base input name, etc)?



Comment at: clang/lib/Driver/Driver.cpp:5541-5542
+  //
+  // FIXME: Do we need to handle the case that the calculated output is
+  // conflicting with the specified output file or the input file?
+  if (!AtTopLevel && isa(JA) &&

tahonermann wrote:
> ChuanqiXu wrote:
> > dblaikie wrote:
> > > Do we do that for `-o` today? (eg: if you try to `-o` and specify the 
> > > input file name, such that the output would overwrite the input, what 
> > > happens? I'm not sure - but I guess it's OK to do whatever that is for 
> > > this case too)
> > > Do we do that for -o today? (eg: if you try to -o and specify the input 
> > > file name, such that the output would overwrite the input, what happens? 
> > > I'm not sure - but I guess it's OK to do whatever that is for this case 
> > > too)
> > 
> > In this case, the input file will be overwrite and no warning shows. So it 
> > looks like we didn't take special treatment here. So I remove the FIXME.
> Basing the location of the module output on the presence of `-o` sounds 
> confusing to me. Likewise, defaulting to writing next to the input file as 
> opposed to the current directory (where a `.o` file is written by default) 
> sounds wrong. I think this option should be handled similarly to `-o`; it 
> should accept a path and:
>   - If an absolute path is provided, write to that location.
>   - If a relative path is provided, write to that location relative to the 
> current working directory.
> Leave it up to the user or build system to ensure that the `.o` and `.pcm` 
> file locations coincide if that is what they want. In general, I don't expect 
> colocation of `.o` and `.pcm` files to be what is desired.
> 
> 
@tahonermann there's precedent for this with Split DWARF (.dwo files go next to 
the .o file) & I'd argued for possibly only providing this behavior, letting 
consumers move files elsewhere if they needed to, but got voted down there.

I do think this is a reasonable default, though. Users and build systems have 
the option to pass a path to place the .pcm somewhere else (in the follow-up 
patch to this one).


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2022-12-12 Thread Tom Honermann via Phabricator via cfe-commits

tahonermann added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:5541-5542
+  //
+  // FIXME: Do we need to handle the case that the calculated output is
+  // conflicting with the specified output file or the input file?
+  if (!AtTopLevel && isa(JA) &&

ChuanqiXu wrote:
> dblaikie wrote:
> > Do we do that for `-o` today? (eg: if you try to `-o` and specify the input 
> > file name, such that the output would overwrite the input, what happens? 
> > I'm not sure - but I guess it's OK to do whatever that is for this case too)
> > Do we do that for -o today? (eg: if you try to -o and specify the input 
> > file name, such that the output would overwrite the input, what happens? 
> > I'm not sure - but I guess it's OK to do whatever that is for this case too)
> 
> In this case, the input file will be overwrite and no warning shows. So it 
> looks like we didn't take special treatment here. So I remove the FIXME.
Basing the location of the module output on the presence of `-o` sounds 
confusing to me. Likewise, defaulting to writing next to the input file as 
opposed to the current directory (where a `.o` file is written by default) 
sounds wrong. I think this option should be handled similarly to `-o`; it 
should accept a path and:
  - If an absolute path is provided, write to that location.
  - If a relative path is provided, write to that location relative to the 
current working directory.
Leave it up to the user or build system to ensure that the `.o` and `.pcm` file 
locations coincide if that is what they want. In general, I don't expect 
colocation of `.o` and `.pcm` files to be what is desired.




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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2022-12-12 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a subscriber: urnathan.
dblaikie added a comment.

(I'd probably still reduce the test down to one example, using `-o` and 
skipping the extra `OUTPUT_PATH` details, only checking the last part of the 
path is as specified (or perhaps checking that it matches the .o file?))

Also, could you consider the questions @urnathan asked in the 
cross-project/flag naming thread about the semantics of this flag - I imagine 
the right behavior is "whatever we do for .o files", but it'd be good to 
check/consider/test them (they may not need automated testing - if the 
behaviour is implemented with the same utilities for .o files as for .pcm files 
then I don't mind just a statement that that's the case and some manual testing 
has been done to verify that all works)?




Comment at: clang/test/Driver/save-std-c++-module-file.cpp:8-9
+//
+// RUN: %clang -std=c++20 %t/Hello.cppm -fmodule-output -o %t/output/Hello.o \
+// RUN:   -### 2>&1 | FileCheck %t/Hello.cppm -DPREFIX=%t 
--check-prefix=CHECK-WITH-OUTPUT
+

ChuanqiXu wrote:
> dblaikie wrote:
> > Not sure I understand the need for two tests here - they both specify an 
> > absolute path to a .o file & CHECK that the absolute path matches the .pcm 
> > output file path, so they don't seem to be testing distinct scenarios?
> The above one doesn't specify the path for the .o file. So in the first test 
> the .pcm file will be in the same directory with the input source file. And 
> in the second command line, the .pcm file will be in the same directory with 
> the .o file.
Ah, maybe just testing the explicit `-o` version is adequate? We aren't doing 
anything interesting here except "whatever the .o path is" - so testing just 
"-o" seems sufficient to test the one path through this code. The pcm code 
doesn't have a "-o" and "implicit -o" codepath, just "do whatever the .o path 
is" so one test seems fine to me?



Comment at: clang/test/Driver/save-std-c++-module-file.cpp:6
+// RUN: %clang -std=c++20 %t/Hello.cppm -fsave-std-c++-module-file -### 2>&1 | 
\
+// RUN:   FileCheck %t/Hello.cppm -DPREFIX=%t
+//

ChuanqiXu wrote:
> dblaikie wrote:
> > Is the prefix needed? I'd expect we'd usually regex match away the actual 
> > directory with `{{.*}}` in tests like this?
> Yeah, generally we would use `{{.*}}`. And `-DPREFIX` is more precise for 
> what we want to test. So I feel it wouldn't be bad.
I think the extra precision probably isn't necessary/sufficiently valuable & we 
could use `{{.*}}` here?


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2022-12-11 Thread Chuanqi Xu via Phabricator via cfe-commits

ChuanqiXu marked an inline comment as done.
ChuanqiXu added inline comments.



Comment at: clang/test/Driver/save-std-c++-module-file.cpp:8-9
+//
+// RUN: %clang -std=c++20 %t/Hello.cppm -fmodule-output -o %t/output/Hello.o \
+// RUN:   -### 2>&1 | FileCheck %t/Hello.cppm -DPREFIX=%t 
--check-prefix=CHECK-WITH-OUTPUT
+

dblaikie wrote:
> Not sure I understand the need for two tests here - they both specify an 
> absolute path to a .o file & CHECK that the absolute path matches the .pcm 
> output file path, so they don't seem to be testing distinct scenarios?
The above one doesn't specify the path for the .o file. So in the first test 
the .pcm file will be in the same directory with the input source file. And in 
the second command line, the .pcm file will be in the same directory with the 
.o file.



Comment at: clang/test/Driver/save-std-c++-module-file.cpp:1-4
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//

dblaikie wrote:
> dblaikie wrote:
> > Is this needed? Maybe we don't need to split the file, if it's just the one 
> > file anyway?
> Ping on this ^
Sorry for missing it. Thanks for clarification.



Comment at: clang/test/Driver/save-std-c++-module-file.cpp:6
+// RUN: %clang -std=c++20 %t/Hello.cppm -fsave-std-c++-module-file -### 2>&1 | 
\
+// RUN:   FileCheck %t/Hello.cppm -DPREFIX=%t
+//

dblaikie wrote:
> Is the prefix needed? I'd expect we'd usually regex match away the actual 
> directory with `{{.*}}` in tests like this?
Yeah, generally we would use `{{.*}}`. And `-DPREFIX` is more precise for what 
we want to test. So I feel it wouldn't be bad.


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2022-12-11 Thread Chuanqi Xu via Phabricator via cfe-commits

ChuanqiXu updated this revision to Diff 481975.
ChuanqiXu marked 3 inline comments as done.
ChuanqiXu edited the summary of this revision.
ChuanqiXu added a comment.

Address comments.


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

https://reviews.llvm.org/D137058

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/lit.local.cfg
  clang/test/Driver/module-output.cppm


Index: clang/test/Driver/module-output.cppm
===
--- /dev/null
+++ clang/test/Driver/module-output.cppm
@@ -0,0 +1,16 @@
+// Tests that the .pcm file will be generated in the same directory with the 
input
+// and the name of the .pcm file should be the same with the input file.
+// RUN: %clang -std=c++20 %s -fmodule-output -c -### 2>&1 | \
+// RUN:   FileCheck %s -DSOURCE_PATH=%S
+//
+// Tests that the .pcm file will be generated in the same direcotry with the 
specified 
+// output and the name of the .pcm file should be the same with the output 
file.
+// RUN: %clang -std=c++20 %s -fmodule-output -c -o %t/output/Hello.o \
+// RUN:   -### 2>&1 | FileCheck %s -DOUTPUT_PATH=%t/output 
--check-prefix=CHECK-WITH-OUTPUT
+
+export module Hello;
+
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" 
"module-output.cppm" {{.*}}"-o" "[[SOURCE_PATH]]/module-output.pcm" "-x" "c++" 
"[[SOURCE_PATH]]/module-output.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "module-output.cppm" {{.*}}"-o" 
"module-output.o" "-x" "pcm" "[[SOURCE_PATH]]/module-output.pcm"
+// CHECK-WITH-OUTPUT: "-emit-module-interface" {{.*}}"-main-file-name" 
"module-output.cppm" {{.*}}"-o" "[[OUTPUT_PATH]]/Hello.pcm" "-x" "c++" 
"{{.*}}/module-output.cppm"
+// CHECK-WITH-OUTPUT: "-emit-obj" {{.*}}"-main-file-name" "module-output.cppm" 
{{.*}}"-o" "[[OUTPUT_PATH]]/Hello.o" "-x" "pcm" "[[OUTPUT_PATH]]/Hello.pcm"
Index: clang/test/Driver/lit.local.cfg
===
--- clang/test/Driver/lit.local.cfg
+++ clang/test/Driver/lit.local.cfg
@@ -1,6 +1,6 @@
 from lit.llvm import llvm_config
 
-config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', 
'.F90', '.f95',
+config.suffixes = ['.c', '.cpp', '.cppm', '.h', '.m', '.mm', '.S', '.s', 
'.f90', '.F90', '.f95',
'.cu', '.rs', '.cl', '.clcpp', '.hip', '.hlsl']
 config.substitutions = list(config.substitutions)
 config.substitutions.insert(0,
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -5529,6 +5529,25 @@
 return "-";
   }
 
+  // If `-fmodule-output` is specfied, then:
+  // - If `-o` is specified, the module file is writing to the same path
+  //   with the output file in module file's suffix. 
+  // - If `-o` is not specified, the module file is writing to the same path
+  //   with the input file in module file's suffix.
+  if (!AtTopLevel && isa(JA) &&
+  JA.getType() == types::TY_ModuleFile &&
+  C.getArgs().hasArg(options::OPT_fmodule_output)) {
+SmallString<128> TempPath;
+if (Arg *FinalOutput = C.getArgs().getLastArg(options::OPT_o))
+  TempPath = FinalOutput->getValue();
+else
+  TempPath = BaseInput;
+
+const char *Extension = types::getTypeTempSuffix(JA.getType());
+llvm::sys::path::replace_extension(TempPath, Extension);
+return C.addResultFile(C.getArgs().MakeArgString(TempPath.c_str()), );
+  }
+
   if (IsDXCMode() && !C.getArgs().hasArg(options::OPT_o))
 return "-";
 
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2285,6 +2285,9 @@
   PosFlag,
   NegFlag, BothFlags<[NoXarchOption, CC1Option]>>;
 
+def fmodule_output : Flag<["-"], "fmodule-output">, Flags<[NoXarchOption, 
CC1Option]>,
+  HelpText<"Save intermediate module file results when compiling a standard 
C++ module unit.">;
+
 def fmodules_prune_interval : Joined<["-"], "fmodules-prune-interval=">, 
Group,
   Flags<[CC1Option]>, MetaVarName<"">,
   HelpText<"Specify the interval (in seconds) between attempts to prune the 
module cache">,


Index: clang/test/Driver/module-output.cppm
===
--- /dev/null
+++ clang/test/Driver/module-output.cppm
@@ -0,0 +1,16 @@
+// Tests that the .pcm file will be generated in the same directory with the input
+// and the name of the .pcm file should be the same with the input file.
+// RUN: %clang -std=c++20 %s -fmodule-output -c -### 2>&1 | \
+// RUN:   FileCheck %s -DSOURCE_PATH=%S
+//
+// Tests that the .pcm file will be generated in the same direcotry with the specified 
+// output and the name of the .pcm file should be the same with the output file.
+// RUN: %clang -std=c++20 %s -fmodule-output -c -o %t/output/Hello.o \

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2022-12-09 Thread H. Vetinari via Phabricator via cfe-commits

h-vetinari added inline comments.



Comment at: clang/test/Driver/save-std-c++-module-file.cpp:1
+// RUN: rm -rf %t
+// RUN: mkdir %t

The filename of this test still reflects the old option name and should 
presumably be changed.


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2022-12-09 Thread David Blaikie via Phabricator via cfe-commits

dblaikie requested changes to this revision.
dblaikie added a comment.
This revision now requires changes to proceed.

Please update the patch description/commit message to reflect the new naming.

Comment at: clang/test/Driver/save-std-c++-module-file.cpp:8-9
+//
+// RUN: %clang -std=c++20 %t/Hello.cppm -fmodule-output -o %t/output/Hello.o \
+// RUN:   -### 2>&1 | FileCheck %t/Hello.cppm -DPREFIX=%t 
--check-prefix=CHECK-WITH-OUTPUT
+

Not sure I understand the need for two tests here - they both specify an 
absolute path to a .o file & CHECK that the absolute path matches the .pcm 
output file path, so they don't seem to be testing distinct scenarios?

Comment at: clang/test/Driver/save-std-c++-module-file.cpp:14-15
+
+// CHECK: "-o" "[[PREFIX]]/Hello.pcm"
+// CHECK-WITH-OUTPUT: "-o" "[[PREFIX]]/output/Hello.pcm"

Might be worth fleshing out the CHECKs a bit more - 

If this patch implements:
> Support .cppm -> .pcm + .o in one phase.

Then I'd expect the test to demonstrate that - show that there are two commands 
run by the driver and one outputs the .o file (so include checking for the .o 
file output file name, and whatever flags tell the frontend that object code is 
to be emitted) and one that outputs the .pcm file (including checking the .pcm 
file output file name, and whatever flags tell the frontend that a pcm should 
be emitted).

Comment at: clang/test/Driver/save-std-c++-module-file.cpp:1-4
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//

dblaikie wrote:
> Is this needed? Maybe we don't need to split the file, if it's just the one 
> file anyway?
Ping on this ^

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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2022-12-09 Thread Chuanqi Xu via Phabricator via cfe-commits

ChuanqiXu added a comment.

In D137058#3983687 , @iains wrote:

> thanks all for the patience on this one - and for the collaborative 
> discussion - I do think the outcome is going to be an easier to remember 
> option name ;)

Yeah. Thanks for your suggestion too. Otherwise I guess we won't get back : )


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2022-12-09 Thread Iain Sandoe via Phabricator via cfe-commits

iains added a comment.

thanks all for the patience on this one - and for the collaborative discussion 
- I do think the outcome is going to be an easier to remember option name ;)


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

2022-12-08 Thread Chuanqi Xu via Phabricator via cfe-commits

ChuanqiXu updated this revision to Diff 481503.
ChuanqiXu marked 2 inline comments as done.
ChuanqiXu retitled this revision from "[Driver] [Modules] Support 
-fsave-std-c++-module-file (1/2)" to "[Driver] [Modules] Support 
-fmodule-output (1/2)".
ChuanqiXu added a comment.

Rename the option into `-fmodule-output` according to the discussion result 
from: https://gcc.gnu.org/pipermail/gcc/2022-December/240239.html


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

https://reviews.llvm.org/D137058

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/save-std-c++-module-file.cpp


Index: clang/test/Driver/save-std-c++-module-file.cpp
===
--- /dev/null
+++ clang/test/Driver/save-std-c++-module-file.cpp
@@ -0,0 +1,15 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang -std=c++20 %t/Hello.cppm -fmodule-output -### 2>&1 | \
+// RUN:   FileCheck %t/Hello.cppm -DPREFIX=%t
+//
+// RUN: %clang -std=c++20 %t/Hello.cppm -fmodule-output -o %t/output/Hello.o \
+// RUN:   -### 2>&1 | FileCheck %t/Hello.cppm -DPREFIX=%t 
--check-prefix=CHECK-WITH-OUTPUT
+
+//--- Hello.cppm
+export module Hello;
+
+// CHECK: "-o" "[[PREFIX]]/Hello.pcm"
+// CHECK-WITH-OUTPUT: "-o" "[[PREFIX]]/output/Hello.pcm"
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -5529,6 +5529,25 @@
 return "-";
   }
 
+  // If `-fmodule-output` is specfied, then:
+  // - If `-o` is specified, the module file is writing to the same path
+  //   with the output file in module file's suffix. 
+  // - If `-o` is not specified, the module file is writing to the same path
+  //   with the input file in module file's suffix.
+  if (!AtTopLevel && isa(JA) &&
+  JA.getType() == types::TY_ModuleFile &&
+  C.getArgs().hasArg(options::OPT_fmodule_output)) {
+SmallString<128> TempPath;
+if (Arg *FinalOutput = C.getArgs().getLastArg(options::OPT_o))
+  TempPath = FinalOutput->getValue();
+else
+  TempPath = BaseInput;
+
+const char *Extension = types::getTypeTempSuffix(JA.getType());
+llvm::sys::path::replace_extension(TempPath, Extension);
+return C.addResultFile(C.getArgs().MakeArgString(TempPath.c_str()), );
+  }
+
   if (IsDXCMode() && !C.getArgs().hasArg(options::OPT_o))
 return "-";
 
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2285,6 +2285,9 @@
   PosFlag,
   NegFlag, BothFlags<[NoXarchOption, CC1Option]>>;
 
+def fmodule_output : Flag<["-"], "fmodule-output">, Flags<[NoXarchOption, 
CC1Option]>,
+  HelpText<"Save intermediate module file results when compiling a standard 
C++ module unit.">;
+
 def fmodules_prune_interval : Joined<["-"], "fmodules-prune-interval=">, 
Group,
   Flags<[CC1Option]>, MetaVarName<"">,
   HelpText<"Specify the interval (in seconds) between attempts to prune the 
module cache">,


Index: clang/test/Driver/save-std-c++-module-file.cpp
===
--- /dev/null
+++ clang/test/Driver/save-std-c++-module-file.cpp
@@ -0,0 +1,15 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang -std=c++20 %t/Hello.cppm -fmodule-output -### 2>&1 | \
+// RUN:   FileCheck %t/Hello.cppm -DPREFIX=%t
+//
+// RUN: %clang -std=c++20 %t/Hello.cppm -fmodule-output -o %t/output/Hello.o \
+// RUN:   -### 2>&1 | FileCheck %t/Hello.cppm -DPREFIX=%t --check-prefix=CHECK-WITH-OUTPUT
+
+//--- Hello.cppm
+export module Hello;
+
+// CHECK: "-o" "[[PREFIX]]/Hello.pcm"
+// CHECK-WITH-OUTPUT: "-o" "[[PREFIX]]/output/Hello.pcm"
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -5529,6 +5529,25 @@
 return "-";
   }
 
+  // If `-fmodule-output` is specfied, then:
+  // - If `-o` is specified, the module file is writing to the same path
+  //   with the output file in module file's suffix. 
+  // - If `-o` is not specified, the module file is writing to the same path
+  //   with the input file in module file's suffix.
+  if (!AtTopLevel && isa(JA) &&
+  JA.getType() == types::TY_ModuleFile &&
+  C.getArgs().hasArg(options::OPT_fmodule_output)) {
+SmallString<128> TempPath;
+if (Arg *FinalOutput = C.getArgs().getLastArg(options::OPT_o))
+  TempPath = FinalOutput->getValue();
+else
+  TempPath = BaseInput;
+
+const char *Extension = types::getTypeTempSuffix(JA.getType());
+llvm::sys::path::replace_extension(TempPath, Extension);
+return C.addResultFile(C.getArgs().MakeArgString(TempPath.c_str()), );
+  }
+
   if (IsDXCMode() &&

54 matches

Mail list logo