<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Feb 29, 2016 at 11:19 AM, Justin Lebar via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">> Can you expand on this? The idea is that those implicit deps are in addition to regular dependencies, not a replacement.<br>
<br>
</span>Sure.<br>
<br>
What I was trying to say is, suppose you have two CUDA device<br>
compilations. You want device compilation B to bail if device<br>
compilation A fails. So what are the implicit dependencies of B?<br>
<br>
Well, B must implicitly depend on everything in A that may trigger a<br>
failure. Which is, I recall, four actions: preprocessor, compilation,<br>
backend, and assemble. It may in fact be one or two more because we<br>
have these CUDADeviceActions that wrap other actions, and maybe we'd<br>
have to depend on those too.<br></blockquote><div><br>Yeah, that doesn't sound quite right - presumably our link action doesn't depend on all those steps, just the last one. If any others fail, the rest can't run (because they depend on them in turn) so the link won't run.<br><br>(but, yeah, I'm no Driver expert by any means..)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Or maybe the thing you're proposing is smart enough that we only need<br>
to depend on the assemble action and it will infer the rest correctly.<br>
Either way, I was just saying that if you add another action, C, it<br>
now has to depend on everything from A and B, and so on.<br>
<span class=""><br>
> One problem I have with this change is that it breaks part of my patch ;-)<br>
<br>
</span>Now it's my turn to admit that I didn't quite follow the rest of this. :)<br>
<span class=""><br>
> if you're in the office tomorrow, maybe we could discuss this for half an hour or so in person?<br>
<br>
</span>Yes, absolutely, let's talk irl and figure this out.<br>
<div class="HOEnZb"><div class="h5"><br>
On Mon, Feb 29, 2016 at 10:43 AM, Nico Weber <<a href="mailto:thakis@chromium.org">thakis@chromium.org</a>> wrote:<br>
> On Sun, Feb 28, 2016 at 3:40 PM, Justin Lebar <<a href="mailto:jlebar@google.com">jlebar@google.com</a>> wrote:<br>
>><br>
>> On Sun, Feb 28, 2016 at 1:46 PM, Nico Weber <<a href="mailto:thakis@chromium.org">thakis@chromium.org</a>> wrote:<br>
>> > Do you think something like the implicit inputs thing in<br>
>> > <a href="http://reviews.llvm.org/D17695" rel="noreferrer" target="_blank">http://reviews.llvm.org/D17695</a> could work for you as well, instead of<br>
>> > this<br>
>> > patch?<br>
>><br>
>> Having read just the patch description, I think it would be workable,<br>
>> although it might not be particularly clean. I think you'd have to<br>
>> make most (all?) of the intermediate actions associated with a<br>
>> compilation be implicit inputs to all later actions in all other<br>
>> compilations. Otherwise things like saving vs. not saving temps would<br>
>> give you different behavior.<br>
><br>
><br>
> Can you expand on this? The idea is that those implicit deps are in addition<br>
> to regular dependencies, not a replacement.<br>
><br>
>><br>
>> It seems like kind of a fragile way to get this behavior.<br>
>><br>
>> > Then we don't have to forever promise to compile all .cc input files<br>
>> > serially.<br>
>><br>
>> I've thought about this some, since we don't really want this even for<br>
>> CUDA. With CUDA, you commonly want to compile the device code for<br>
>> different architectures, and it would be reasonable to do those<br>
>> compilations in parallel.<br>
>><br>
>> What I think may be a reasonable promise, at least for CUDA, is that<br>
>> we will behave *as if* we're compiling in series. Which just means<br>
>> not interleaving diagnostics from different compilations, and not<br>
>> showing diagnostics from other subcompilations after we hit an error.<br>
>><br>
>> I think basically nobody wants randomly-interleaved diagnostics.<br>
>> Whether or there's a good use-case where we continue after we hit an<br>
>> error is a harder question, I'm not sure. But in any case, stopping<br>
>> after an error shouldn't mean we're forced to serialize, I think.<br>
><br>
><br>
> One problem I have with this change is that it breaks part of my patch ;-)<br>
> I'd like to run two commands, one to build a pch, and another to compile a<br>
> source file using the just-built pch, and if the first fails I don't want to<br>
> run the second: `make_pch && compile`. This part still works after your<br>
> patch. But we also have a flag /fallback that says "if compilation falls,<br>
> try again with this other compiler. Ideally I want `(make_pch && compile) ||<br>
> fallback_compile`, but since it's a bit of a corner case and arguably good<br>
> enough, my patch did `make_pch ; compile || fallback`. Your change makes it<br>
> impossible to run commands after one another if one fails, so now this is<br>
> `make_pch && (compile || fallback)`, i.e. if compilation of the pch fails<br>
> the fallback compiler won't be invoked.<br>
><br>
> I can try looking at maybe making FallbackCommand a FallbackAction instead<br>
> or something. But since CUDA seems to not fit the internal model of the<br>
> driver super well (lots of isCUDA() calls in many places), maybe it'd make<br>
> sense to discuss what your requirements are and if it's possible to extend<br>
> the Action / Job / Command abstractions in a way that support CUDA without<br>
> as many special cases. I happen to be in Mountain View – if you're in the<br>
> office tomorrow, maybe we could discuss this for half an hour or so in<br>
> person?<br>
><br>
>><br>
>><br>
>> -Justin<br>
>><br>
>> > On Wed, Feb 24, 2016 at 4:49 PM, Justin Lebar via cfe-commits<br>
>> > <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br>
>> >><br>
>> >> Author: jlebar<br>
>> >> Date: Wed Feb 24 15:49:28 2016<br>
>> >> New Revision: 261774<br>
>> >><br>
>> >> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=261774&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=261774&view=rev</a><br>
>> >> Log:<br>
>> >> Bail on compilation as soon as a job fails.<br>
>> >><br>
>> >> Summary:<br>
>> >> (Re-land of r260448, which was reverted in r260522 due to a test<br>
>> >> failure<br>
>> >> in Driver/output-file-cleanup.c that only showed up in fresh builds.)<br>
>> >><br>
>> >> Previously we attempted to be smart; if one job failed, we'd run all<br>
>> >> jobs that didn't depend on the failing job.<br>
>> >><br>
>> >> Problem is, this doesn't work well for e.g. CUDA compilation without<br>
>> >> -save-temps. In this case, the device-side and host-side Assemble<br>
>> >> actions (which actually are responsible for preprocess, compile,<br>
>> >> backend, and assemble, since we're not saving temps) are necessarily<br>
>> >> distinct. So our clever heuristic doesn't help us, and we repeat every<br>
>> >> error message once for host and once for each device arch.<br>
>> >><br>
>> >> The main effect of this change, other than fixing CUDA, is that if you<br>
>> >> pass multiple cc files to one instance of clang and you get a compile<br>
>> >> error, we'll stop when the first cc1 job fails.<br>
>> >><br>
>> >> Reviewers: echristo<br>
>> >><br>
>> >> Subscribers: cfe-commits, jhen, echristo, tra, rafael<br>
>> >><br>
>> >> Differential Revision: <a href="http://reviews.llvm.org/D17217" rel="noreferrer" target="_blank">http://reviews.llvm.org/D17217</a><br>
>> >><br>
>> >> Modified:<br>
>> >> cfe/trunk/lib/Driver/Compilation.cpp<br>
>> >> cfe/trunk/test/Driver/output-file-cleanup.c<br>
>> >><br>
>> >> Modified: cfe/trunk/lib/Driver/Compilation.cpp<br>
>> >> URL:<br>
>> >><br>
>> >> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Compilation.cpp?rev=261774&r1=261773&r2=261774&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Compilation.cpp?rev=261774&r1=261773&r2=261774&view=diff</a><br>
>> >><br>
>> >><br>
>> >> ==============================================================================<br>
>> >> --- cfe/trunk/lib/Driver/Compilation.cpp (original)<br>
>> >> +++ cfe/trunk/lib/Driver/Compilation.cpp Wed Feb 24 15:49:28 2016<br>
>> >> @@ -163,39 +163,17 @@ int Compilation::ExecuteCommand(const Co<br>
>> >> return ExecutionFailed ? 1 : Res;<br>
>> >> }<br>
>> >><br>
>> >> -typedef SmallVectorImpl< std::pair<int, const Command *> ><br>
>> >> FailingCommandList;<br>
>> >> -<br>
>> >> -static bool ActionFailed(const Action *A,<br>
>> >> - const FailingCommandList &FailingCommands) {<br>
>> >> -<br>
>> >> - if (FailingCommands.empty())<br>
>> >> - return false;<br>
>> >> -<br>
>> >> - for (FailingCommandList::const_iterator CI =<br>
>> >> FailingCommands.begin(),<br>
>> >> - CE = FailingCommands.end(); CI != CE; ++CI)<br>
>> >> - if (A == &(CI->second->getSource()))<br>
>> >> - return true;<br>
>> >> -<br>
>> >> - for (const Action *AI : A->inputs())<br>
>> >> - if (ActionFailed(AI, FailingCommands))<br>
>> >> - return true;<br>
>> >> -<br>
>> >> - return false;<br>
>> >> -}<br>
>> >> -<br>
>> >> -static bool InputsOk(const Command &C,<br>
>> >> - const FailingCommandList &FailingCommands) {<br>
>> >> - return !ActionFailed(&C.getSource(), FailingCommands);<br>
>> >> -}<br>
>> >> -<br>
>> >> -void Compilation::ExecuteJobs(const JobList &Jobs,<br>
>> >> - FailingCommandList &FailingCommands)<br>
>> >> const<br>
>> >> {<br>
>> >> +void Compilation::ExecuteJobs(<br>
>> >> + const JobList &Jobs,<br>
>> >> + SmallVectorImpl<std::pair<int, const Command *>> &FailingCommands)<br>
>> >> const {<br>
>> >> for (const auto &Job : Jobs) {<br>
>> >> - if (!InputsOk(Job, FailingCommands))<br>
>> >> - continue;<br>
>> >> const Command *FailingCommand = nullptr;<br>
>> >> - if (int Res = ExecuteCommand(Job, FailingCommand))<br>
>> >> + if (int Res = ExecuteCommand(Job, FailingCommand)) {<br>
>> >> FailingCommands.push_back(std::make_pair(Res, FailingCommand));<br>
>> >> + // Bail as soon as one command fails, so we don't output<br>
>> >> duplicate<br>
>> >> error<br>
>> >> + // messages if we die on e.g. the same file.<br>
>> >> + return;<br>
>> >> + }<br>
>> >> }<br>
>> >> }<br>
>> >><br>
>> >><br>
>> >> Modified: cfe/trunk/test/Driver/output-file-cleanup.c<br>
>> >> URL:<br>
>> >><br>
>> >> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/output-file-cleanup.c?rev=261774&r1=261773&r2=261774&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/output-file-cleanup.c?rev=261774&r1=261773&r2=261774&view=diff</a><br>
>> >><br>
>> >><br>
>> >> ==============================================================================<br>
>> >> --- cfe/trunk/test/Driver/output-file-cleanup.c (original)<br>
>> >> +++ cfe/trunk/test/Driver/output-file-cleanup.c Wed Feb 24 15:49:28<br>
>> >> 2016<br>
>> >> @@ -38,6 +38,9 @@ invalid C code<br>
>> >> // RUN: test -f %t1.s<br>
>> >> // RUN: test ! -f %t2.s<br>
>> >><br>
>> >> +// When given multiple .c files to compile, clang compiles them in<br>
>> >> order<br>
>> >> until<br>
>> >> +// it hits an error, at which point it stops.<br>
>> >> +//<br>
>> >> // RUN: touch %t1.c<br>
>> >> // RUN: echo "invalid C code" > %t2.c<br>
>> >> // RUN: touch %t3.c<br>
>> >> @@ -46,6 +49,6 @@ invalid C code<br>
>> >> // RUN: cd %T && not %clang -S %t1.c %t2.c %t3.c %t4.c %t5.c<br>
>> >> // RUN: test -f %t1.s<br>
>> >> // RUN: test ! -f %t2.s<br>
>> >> -// RUN: test -f %t3.s<br>
>> >> +// RUN: test ! -f %t3.s<br>
>> >> // RUN: test ! -f %t4.s<br>
>> >> -// RUN: test -f %t5.s<br>
>> >> +// RUN: test ! -f %t5.s<br>
>> >><br>
>> >><br>
>> >> _______________________________________________<br>
>> >> cfe-commits mailing list<br>
>> >> <a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
>> >> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
>> ><br>
>> ><br>
><br>
><br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br></div></div>