<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jul 16, 2015 at 3:28 PM, Justin Bogner <span dir="ltr"><<a href="mailto:mail@justinbogner.com" target="_blank">mail@justinbogner.com</a>></span> wrote:</div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div>> > + CudaInjectionPhase = *i;<br>
> > + }<br>
> > + } else<br>
> > + CudaInjectionPhase = FinalPhase;<br>
> > + }<br>
> > +<br>
> > // Build the pipeline for this file.<br>
> > std::unique_ptr<Action> Current(new InputAction(*InputArg,<br>
> InputType));<br>
> > for (SmallVectorImpl<phases::ID>::iterator i = PL.begin(), e =<br>
> PL.end();<br>
> > @@ -1337,6 +1456,15 @@ void Driver::BuildActions(const ToolChai<br>
> ><br>
> > // Otherwise construct the appropriate action.<br>
> > Current = ConstructPhaseAction(TC, Args, Phase, std::move<br>
> (Current));<br>
> > +<br>
> > + if (InputType == types::TY_CUDA && Phase == CudaInjectionPhase &&<br>
> > + !Args.hasArg(options::OPT_cuda_host_only)) {<br>
> > + Current = buildCudaActions(*this, TC, Args, InputArg,<br>
> InputType,<br>
> > + std::move(Current), Actions);<br>
> > + if (!Current)<br>
> > + break;<br>
> > + }<br>
><br>
> This whole block of code seems out of place. In fact, why doesn't CUDA<br>
> just have its own phases::ID? It seems like that would simplify all of<br>
> this.<br>
><br>
> It does not quite fit the 'phase' concept. Phases implicitly assume that<br>
> things are done linearly. CUDA, on the other hand, effectively builds a tree<br>
> and the weird code above is a somewhat broken way to glue results of<br>
> device-side builds into just the right action of the host build pipeline, so<br>
> that results can be used to modify compiler arguments.<br>
<br>
</div></div>Well, I wouldn't say they assume things are done linearly - the phases<br>
are already used to build trees today. The link phase, for example, will<br>
depend on multiple different compile branches.<br>
<br></blockquote><div><br></div><div>Let me rephrase -- selectToolForJob() assumes that actions are a arranged in a linear list and that it can short-circuit traversal assuming that fact.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
The difference here is just that we want multiple branches for the same<br>
input file (which means we end up with a DAG instead of a tree I<br>
suppose). </blockquote><div><br></div><div>Current implementation supplies input source file to device actions independently, so Action graph is still a tree, though it could be made a DAG. It does not really matter in this case, though. A lot of code in lib/Driver assumes (sometimes explicitly with an assert, but often implicitly by ignoring inputs other than the very first one) that we ever deal with one input only. There are few exceptions but by and large they are just that -- exceptions. CUDA case is complicated by the fact that device side output does require special handling.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">I guess we don't have much infrastructure to support this more<br>
cleanly right now - I see that BindArchActions are already handled in a<br>
similarly awkward way.<br>
<span><br>
> I've got a better way of dealing with that that I'll send as a separate patch<br>
> for review soon.<br>
<br>
</span>Looking forward to it.<br></blockquote><div><br></div><div>See <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D11280&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=j2pY1CdJQzoEiutO-67J2EZwp-46K68uwWDPOLtZIos&s=2JfP2RcU0aQqyMzpoVGd8ae_w3NT6zg_I2D7ZsP4C3k&e=" target="_blank">http://reviews.llvm.org/D11280</a></div><div>A've added you to the reviewer list.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div><br>
> > +<br>
> > if (Current->getType() == types::TY_Nothing)<br>
> > break;<br>
> > }<br>
> > @@ -1576,7 +1704,13 @@ static const Tool *SelectToolForJob(Comp<br>
> > if (isa<BackendJobAction>(JA)) {<br>
> > // Check if the compiler supports emitting LLVM IR.<br>
> > assert(Inputs->size() == 1);<br>
> > - JobAction *CompileJA = cast<CompileJobAction>(*Inputs->begin());<br>
> > + JobAction *CompileJA;<br>
> > + // Extract real host action, if it's a CudaHostAction.<br>
> > + if (CudaHostAction *CudaHA = dyn_cast<CudaHostAction>(*Inputs-><br>
> begin()))<br>
> > + CompileJA = cast<CompileJobAction>(*CudaHA->begin());<br>
> > + else<br>
> > + CompileJA = cast<CompileJobAction>(*Inputs->begin());<br>
><br>
> This seems kind of awkward. Not sure I have a better suggestion though.<br>
><br>
> This will also be changes in the upcoming patch.<br>
><br>
> <br>
><br>
> > if (const InputAction *IA = dyn_cast<InputAction>(A)) {<br>
> > // FIXME: It would be nice to not claim this here; maybe the old<br>
> scheme of<br>
> > // just using Args was better?<br>
> > @@ -1635,11 +1783,24 @@ void Driver::BuildJobsForAction(Compilat<br>
> > else<br>
> > TC = &C.getDefaultToolChain();<br>
> ><br>
> > - BuildJobsForAction(C, *BAA->begin(), TC, BAA->getArchName(),<br>
> AtTopLevel,<br>
> > + BuildJobsForAction(C, *BAA->begin(), TC, ArchName, AtTopLevel,<br>
><br>
> I guess this is just an unrelated cleanup? Better to commit those<br>
> separately in the future.<br>
><br>
> Noted.<br>
><br>
> <br>
><br>
> > MultipleArchs, LinkingOutput, Result);<br>
> > return;<br>
> > }<br>
> ><br>
> > + if (const CudaDeviceAction *CDA = dyn_cast<CudaDeviceAction>(A)) {<br>
> > + // Figure out which NVPTX triple to use for device-side compilation<br>
> based on<br>
> > + // whether host is 64-bit.<br>
> > + llvm::Triple DeviceTriple(C.getDefaultToolChain().getTriple<br>
> ().isArch64Bit()<br>
> > + ? "nvptx64-nvidia-cuda"<br>
> > + : "nvptx-nvidia-cuda");<br>
><br>
> It seems like a pretty bad layering violation to need to know these<br>
> particular triples right here...<br>
><br>
> Well, it's up to driver to figure out device-side triple. While I agree that<br>
> it's way too much target knowledge for the driver, I'm short on practical<br>
> ideas. Do you have any suggestions?<br>
<br>
</div></div>I guess I might expect this to be figured out in buildCudaActions, near<br>
where we figure out the device architectures. I guess this triple is<br>
more based on the host-side target though?<br></blockquote><div><br></div><div>Device triple is assumed to be a variant of NVPTX matching 32/64-bitness of the host.</div><div>I guess I can compute the triple and stash it in CudaDeviceAction along with GPU arch.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
This is already a little bit broken, but ideally BuildJobsForAction<br>
would just do the tree traversal, and the details about how to do that<br>
are abstracted elsewhere. </blockquote><div><br></div><div>Makes sense. I'll move triple calculation to buildCudaActions.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">I guess we just don't have a very good story<br>
about how to deal with multiple toolchains in the same build pipeline<br>
today.<br>
<div><div><br></div></div></blockquote><div><br></div><div><br></div></div>-- <br><div><div dir="ltr">--Artem Belevich</div></div>
</div></div>