[BatchMode] Consolidate reasoning about -{enable,disable}-batch-mode flag. #15749
Conversation
…flag.
Previous to this change, the initial inspection of the -{enable,disable}-batch-mode
flag was made separate from the subsequent overriding by -wmo; this in turn meant
that the driver might decide it's "in batch mode" in one place (in particular,
when judging whether to ignore the -num-threads flag) and "in wmo mode" elsewhere
(when judging whether to emit one or multiple outputs, which depends on the
-num-threads flag).
Divergence between these two views caused the driver to effectively drop
the -num-threads flag even when overriding batch mode with wmo; since xcode
assumes that passing -wmo -num-threads _will_ cause multiple outputs, this
in turn caused linking to fail since the expected output files were not found.
| @@ -389,8 +389,11 @@ class Driver { | |||
| /// there is an actual conflict. | |||
| /// \param Args The input arguments. | |||
| /// \param Inputs The inputs to the driver. | |||
| /// \param BatchModeOut An out-parameter flag that indicates whether to | |||
| /// batch the jobs of the resulting \c Mode::StandardCompile compilation. | |||
There was a problem hiding this comment.
This comment is good because it will help explain why this routine does not return Mode::BatchMode for the batch case. Probably would be good to add a comment somewhere (Driver.h 68?) explaining the difference between BatchMode being set and the output mode, sigh. Amazing how code can seem less clear with a bit of time.
lib/Driver/Driver.cpp
Outdated
|
|
||
| return OutputInfo::Mode::SingleCompile; | ||
| return OutputInfo::Mode::StandardCompile; |
There was a problem hiding this comment.
If you like, you could clarify the code a smidge by changing 1406 to if (!ArgRequiringSingleCompile) and then have an immediate return StandardCompile after that if.
There was a problem hiding this comment.
That's what it used to do; I inverted the logic while I was trying to figure out the best way to factor this, and then realized I thought it read better that way so left it as-is. Do you feel strongly about my inverting it again?
There was a problem hiding this comment.
Not strongly enough to insist, but I'm not following your reasoning. I like the original way better because it shortens the average length of the mental stack for someone trying to understand what is returned in what circumstance. The original way is one line for one option, 4-ish lines of code for the other. Average: 2.5. The new way is requires one to read past the single-compile case to get to the standard compile case--an average of about 4-ish lines.
In addition, the new way introduces nested control, vs flatter control the old way.
That's my reasoning. I bet you see a different aspect that is really there that sways you the other way. What is it?
There was a problem hiding this comment.
Doesn't matter. Just want to land this. I'll change it back.
There was a problem hiding this comment.
OK, tx. Would still like to understand what you see there that I don't. I believe it's real.
| // RUN: %swiftc_driver %t/a.swift %t/b.swift %t/c.swift -num-threads 4 -whole-module-optimization -enable-batch-mode -### >%t/stdout_mt_wmo 2>%t/stderr_mt_wmo | ||
| // RUN: %FileCheck --check-prefix CHECK-WMO %s <%t/stderr_mt_wmo | ||
| // RUN: %FileCheck --check-prefix CHECK-MULTITHREADED-WMO-ARGS %s <%t/stdout_mt_wmo | ||
| // CHECK-MULTITHREADED-WMO-ARGS: -num-threads 4 {{.*}}-o {{.*}}/a-{{[a-z0-9]+}}.o -o {{.*}}/b-{{[a-z0-9]+}}.o -o {{.*}}/c-{{[a-z0-9]+}}.o |
|
@davidungar pushed changes addressing review |
| /// having OutputInfo::CompilerMode == StandardCompile, with the | ||
| /// Compilation::BatchModeEnabled flag set to true, _not_ as a | ||
| /// BatchModeCompile Compilation. The top-level OutputInfo::CompilerMode for | ||
| /// a Compilation should never be BatchModeCompile. |
|
@swift-ci please test and merge |
Previous to this change, the initial inspection of the -{enable,disable}-batch-mode flag was made separate from the subsequent overriding by -wmo; this in turn meant that the driver might decide it's "in batch mode" in one place (in particular, when judging whether to ignore the -num-threads flag) and "in wmo mode" elsewhere (when judging whether to emit one or multiple outputs, which depends on the -num-threads flag).
Divergence between these two views caused the driver to effectively drop the -num-threads flag even when overriding batch mode with wmo; since xcode assumes that passing -wmo -num-threads will cause multiple outputs, this in turn caused linking to fail since the expected output files were not found.
rdar://39191323