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

ChuanqiXu added a comment. In D137058#4058836 , @Jake-Egan wrote: > In D137058#4057424 , @ChuanqiXu > wrote: > >> In D137058#4056647 , @Jake-Egan >> wrote: >> >>> Hi,

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

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

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

Jake-Egan added a comment. In D137058#4057424 , @ChuanqiXu wrote: > In D137058#4056647 , @Jake-Egan > wrote: > >> Hi, this new test fails on AIX >>

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

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:

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

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/

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

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

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

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

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

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

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

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;

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

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:

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

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:

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

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: > >

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

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;

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

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:

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

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: > >

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

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: > >

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

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

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

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;

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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:

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

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. > >>

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

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: >

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

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: >

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

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

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

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

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

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

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

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

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

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:

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

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

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

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

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

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) &&

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

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

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

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) &&

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

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) &&

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

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

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

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

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

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

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

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) &&

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

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,

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

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

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

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:

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

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

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

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:

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

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

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

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

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

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