<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body dir="auto">
Even in this case the patches must be splitted per clang/LLVM coding standard.<br>
<br>
<div id="AppleMailSignature" dir="ltr">Best regards,
<div>Alexey Bataev</div>
</div>
<div dir="ltr"><br>
13 марта 2019 г., в 17:55, Doerfert, Johannes <<a href="mailto:jdoerfert@anl.gov">jdoerfert@anl.gov</a>> написал(а):<br>
<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;font-family:Calibri,Helvetica,sans-serif;" dir="ltr">
<p style="margin-top:0;margin-bottom:0">Maybe it helps that I (or better Jose Diaz) run the OpenMP target offloading V&V test suite [1] with the new code generation enabled.</p>
<p style="margin-top:0;margin-bottom:0">All but one test did pass, I'm looking into the remaining problem right now.<br>
</p>
<p style="margin-top:0;margin-bottom:0"><br>
</p>
<p style="margin-top:0;margin-bottom:0">[1] <a href="https://crpl.cis.udel.edu/ompvvsollve/" class="OWAAutoLink" id="LPlnk168335" previewremoved="true">
https://crpl.cis.udel.edu/ompvvsollve/</a><br>
</p>
</div>
<hr style="display:inline-block;width:98%" tabindex="-1">
<div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" style="font-size:11pt" color="#000000"><b>From:</b> Alexey Bataev <<a href="mailto:a.bataev@hotmail.com">a.bataev@hotmail.com</a>><br>
<b>Sent:</b> Wednesday, March 13, 2019 4:33:03 PM<br>
<b>To:</b> Doerfert, Johannes<br>
<b>Cc:</b> Alexey Bataev; <a href="mailto:cfe-dev@lists.llvm.org">cfe-dev@lists.llvm.org</a>;
<a href="mailto:openmp-dev@lists.llvm.org">openmp-dev@lists.llvm.org</a>; llvm-dev; Finkel, Hal J.<br>
<b>Subject:</b> Re: [RFC] Late (OpenMP) GPU code "SPMD-zation"</font>
<div> </div>
</div>
<div class="BodyFragment"><font size="2"><span style="font-size:11pt;">
<div class="PlainText">1. You don't need to implement everything in a single patch. The development process is a step-by-step process, when you commit something in small pieces. The code must nit be fully functional, you may start from some basic features.
Currently it is very hard to review.<br>
2. I rather doubt that it can be reused without changes for AMD etc., especially without being fully tested. The only tested target is NVPTX and at first we need to support it. Later, we could extend it to AMD and some other targets.<br>
3. No, it is not incidental. It is thoroughly tested, at least.<br>
4. Hm, if it would be so, I would just ignored it. Yes, I'm a bit sceptical, but this is normal. It is the fact that these patches break Coding standard, which suggests to split patches into small pieces and commit them one by one.<br>
<br>
Best regards,<br>
Alexey Bataev<br>
<br>
> 13 марта 2019 г., в 17:18, Doerfert, Johannes <<a href="mailto:jdoerfert@anl.gov">jdoerfert@anl.gov</a>> написал(а):<br>
> <br>
>> On 03/13, Alexey Bataev wrote:<br>
>> 13.03.2019 15:35, Doerfert, Johannes пишет:<br>
>>> <br>
>>> Hi Alexey,<br>
>>> <br>
>>> <br>
>>> thank you for your quick feedback.<br>
>>> <br>
>>> <br>
>>>> There are tooooooo(!) many changes, I don't who's going to review sooooo big<br>
>>> patch.<br>
>>> <br>
>>> <br>
>>> I can for sure split it in the three components/repositories that are<br>
>>> touched, clang, llvm, and openmp.<br>
>>> <br>
>>> I feared it will then be harder to navigate the code in order to see<br>
>>> the connection points.<br>
>>> <br>
>>> I am a bit amazed by your hyperbolism though given the complexity is<br>
>>> not that height<br>
>>> <br>
>>> due to the absence of modified or removed lines. Anyway, you seem to<br>
>>> have very strong feelings about<br>
>>> <br>
>>> this so I am open to suggestion on how to split it up.<br>
>>> <br>
>>> <br>
>> <br>
>> 1. You definitely need to split it into separate patches for different<br>
>> components.<br>
> <br>
> Done:<br>
> OpenMP: <a href="https://reviews.llvm.org/D59319">https://reviews.llvm.org/D59319</a><br>
> Clang: <a href="https://reviews.llvm.org/D59328">https://reviews.llvm.org/D59328</a><br>
> LLVM: <a href="https://reviews.llvm.org/D59331">https://reviews.llvm.org/D59331</a><br>
> <br>
>> 2. Even inside of those components this patch must be split into several<br>
>> small patches, it is very hard to review so big patches.<br>
> <br>
> Please take a look at the three patches above. <br>
> <br>
> The first contains the interface definition and implementation for NVPTX<br>
> (in cuda). I don't know how to further split that except to separate it<br>
> into the definition and the implementation, though that does not make<br>
> sense to me.<br>
> <br>
> The second contains the code generation. It is very much like the NVPTX<br>
> code generation except that it does not contain logic.<br>
> <br>
> The third is the LLVM pass which could be split into two, SPMD-mode and<br>
> state machine creation. I'll wait for feedback on the other patches<br>
> until I go ahead.<br>
> <br>
> <br>
>>>> Also, I don't like the idea adding of one more class for NVPTX<br>
>>> codegen. All your changes should be on top of the eixisting solution.<br>
>>> <br>
>>> <br>
>>> Could you please explain to me why? This will only make everything<br>
>>> more complicated and entangled.<br>
>>> Also, the new class is supposed to be "target agnostic" so a new<br>
>>> offloading target, e.g., AMD GPUs, could easily reuse<br>
>>> the new code while the old code is sprinkled with NVPTX specific<br>
>>> details, e.g., function calls, constants, etc.<br>
>>> <br>
>> 1. As far as I know, even now the NVPTX codegen can be reused for AMD<br>
>> GPUs with some small changes.<br>
> <br>
> The target region code generation is supposed to be reusable for<br>
> AMD/XYZ/... without changes.<br>
> <br>
> <br>
>> 2. Your patch is about codegen for NVPTX, so you must change the<br>
>> existing codegen, but not to introduce the new one for the same target.<br>
> <br>
> I strongly disagree. The patch is not "for NVPTX" but for "OpenMP target<br>
> offloading", maybe with a focus on "GPU kernels". The fact that the only<br>
> target offloading device we currently support is based on Cuda and NVPTX<br>
> is incidental.<br>
> <br>
> <br>
>> There is no point to maintain two different codegens for one target.<br>
> <br>
> Given your comments on my initial RFC and prototype I strongly suspected<br>
> you do not want this approach to replace the current NVPTX code<br>
> generation. Once that changes we can get rid of one of them.<br>
> <br>
> <br>
>>> <br>
>>> Thanks again,<br>
>>> Johannes<br>
>>> ------------------------------------------------------------------------<br>
>>> *From:* Alexey Bataev <<a href="mailto:a.bataev@outlook.com">a.bataev@outlook.com</a>><br>
>>> *Sent:* Wednesday, March 13, 2019 2:15:39 PM<br>
>>> *To:* Doerfert, Johannes; <a href="mailto:cfe-dev@lists.llvm.org">cfe-dev@lists.llvm.org</a><br>
>>> *Cc:* <a href="mailto:openmp-dev@lists.llvm.org">openmp-dev@lists.llvm.org</a>; LLVM-Dev; Finkel, Hal J.; Alexey<br>
>>> Bataev; Arpith Chacko Jacob<br>
>>> *Subject:* Re: [RFC] Late (OpenMP) GPU code "SPMD-zation"<br>
>>> <br>
>>> <br>
>>> There are tooooooo(!) many changes, I don't who's going to review<br>
>>> sooooo big patch. You definitely need to split it into several smaller<br>
>>> patches. Also, I don't like the idea adding of one more class for<br>
>>> NVPTX codegen. All your changes should be on top of the eixisting<br>
>>> solution.<br>
>>> <br>
>>> -------------<br>
>>> Best regards,<br>
>>> Alexey Bataev<br>
>>> 13.03.2019 15:08, Doerfert, Johannes пишет:<br>
>>>> Please consider reviewing the code for the proposed approach here:<br>
>>>> <a href="https://reviews.llvm.org/D57460">https://reviews.llvm.org/D57460</a><br>
>>>> <br>
>>>> Initial tests, e.g., on the nw (needleman-wunsch) benchmark in the<br>
>>>> rodinia 3.1 benchmark suite, showed 30% improvement after SPMD mode was<br>
>>>> enabled automatically. The code in nw is conceptually equivalent to the<br>
>>>> first example in the "to_SPMD_mode.ll" test case that can be found here:<br>
>>>> <a href="https://reviews.llvm.org/D57460#change-sBfg7kuN4Bid">https://reviews.llvm.org/D57460#change-sBfg7kuN4Bid</a><br>
>>>> <br>
>>>> The implementation is missing key features but one should be able to see<br>
>>>> the overall design by now. Once accepted, the missing features and more<br>
>>>> optimizations will be added.<br>
>>>> <br>
>>>> <br>
>>>>> On 01/22, Johannes Doerfert wrote:<br>
>>>>> Where we are<br>
>>>>> ------------<br>
>>>>> <br>
>>>>> Currently, when we generate OpenMP target offloading code for GPUs, we<br>
>>>>> use sufficient syntactic criteria to decide between two execution modes:<br>
>>>>> 1) SPMD -- All target threads (in an OpenMP team) run all the code.<br>
>>>>> 2) "Guarded" -- The master thread (of an OpenMP team) runs the user<br>
>>>>> code. If an OpenMP distribute region is encountered, thus<br>
>>>>> if all threads (in the OpenMP team) are supposed to<br>
>>>>> execute the region, the master wakes up the idling<br>
>>>>> worker threads and points them to the correct piece of<br>
>>>>> code for distributed execution.<br>
>>>>> <br>
>>>>> For a variety of reasons we (generally) prefer the first execution mode.<br>
>>>>> However, depending on the code, that might not be valid, or we might<br>
>>>>> just not know if it is in the Clang code generation phase.<br>
>>>>> <br>
>>>>> The implementation of the "guarded" execution mode follows roughly the<br>
>>>>> state machine description in [1], though the implementation is different<br>
>>>>> (more general) nowadays.<br>
>>>>> <br>
>>>>> <br>
>>>>> What we want<br>
>>>>> ------------<br>
>>>>> <br>
>>>>> Increase the amount of code executed in SPMD mode and the use of<br>
>>>>> lightweight "guarding" schemes where appropriate.<br>
>>>>> <br>
>>>>> <br>
>>>>> How we get (could) there<br>
>>>>> ------------------------<br>
>>>>> <br>
>>>>> We propose the following two modifications in order:<br>
>>>>> <br>
>>>>> 1) Move the state machine logic into the OpenMP runtime library. That<br>
>>>>> means in SPMD mode all device threads will start the execution of<br>
>>>>> the user code, thus emerge from the runtime, while in guarded mode<br>
>>>>> only the master will escape the runtime and the other threads will<br>
>>>>> idle in their state machine code that is now just "hidden".<br>
>>>>> <br>
>>>>> Why:<br>
>>>>> - The state machine code cannot be (reasonably) optimized anyway,<br>
>>>>> moving it into the library shouldn't hurt runtime but might even<br>
>>>>> improve compile time a little bit.<br>
>>>>> - The change should also simplify the Clang code generation as we<br>
>>>>> would generate structurally the same code for both execution modes<br>
>>>>> but only the runtime library calls, or their arguments, would<br>
>>>>> differ between them.<br>
>>>>> - The reason we should not "just start in SPMD mode" and "repair"<br>
>>>>> it later is simple, this way we always have semantically correct<br>
>>>>> and executable code.<br>
>>>>> - Finally, and most importantly, there is now only little<br>
>>>>> difference (see above) between the two modes in the code<br>
>>>>> generated by clang. If we later analyze the code trying to decide<br>
>>>>> if we can use SPMD mode instead of guarded mode the analysis and<br>
>>>>> transformation becomes much simpler.<br>
>>>>> <br>
>>>>> 2) Implement a middle-end LLVM-IR pass that detects the guarded mode,<br>
>>>>> e.g., through the runtime library calls used, and that tries to<br>
>>>>> convert it into the SPMD mode potentially by introducing lightweight<br>
>>>>> guards in the process.<br>
>>>>> <br>
>>>>> Why:<br>
>>>>> - After the inliner, and the canonicalizations, we have a clearer<br>
>>>>> picture of the code that is actually executed in the target<br>
>>>>> region and all the side effects it contains. Thus, we can make an<br>
>>>>> educated decision on the required amount of guards that prevent<br>
>>>>> unwanted side effects from happening after a move to SPMD mode.<br>
>>>>> - At this point we can more easily introduce different schemes to<br>
>>>>> avoid side effects by threads that were not supposed to run. We<br>
>>>>> can decide if a state machine is needed, conditionals should be<br>
>>>>> employed, masked instructions are appropriate, or "dummy" local<br>
>>>>> storage can be used to hide the side effect from the outside<br>
>>>>> world.<br>
>>>>> <br>
>>>>> <br>
>>>>> None of this was implemented yet but we plan to start in the immediate<br>
>>>>> future. Any comments, ideas, criticism is welcome!<br>
>>>>> <br>
>>>>> <br>
>>>>> Cheers,<br>
>>>>> Johannes<br>
>>>>> <br>
>>>>> <br>
>>>>> P.S. [2-4] Provide further information on implementation and features.<br>
>>>>> <br>
>>>>> [1] <a href="https://ieeexplore.ieee.org/document/7069297">https://ieeexplore.ieee.org/document/7069297</a><br>
>>>>> [2] <a href="https://dl.acm.org/citation.cfm?id=2833161">https://dl.acm.org/citation.cfm?id=2833161</a><br>
>>>>> [3] <a href="https://dl.acm.org/citation.cfm?id=3018870">https://dl.acm.org/citation.cfm?id=3018870</a><br>
>>>>> [4] <a href="https://dl.acm.org/citation.cfm?id=3148189">https://dl.acm.org/citation.cfm?id=3148189</a><br>
>>>>> <br>
>>>>> <br>
>>>>> -- <br>
>>>>> <br>
>>>>> Johannes Doerfert<br>
>>>>> Researcher<br>
>>>>> <br>
>>>>> Argonne National Laboratory<br>
>>>>> Lemont, IL 60439, USA<br>
>>>>> <br>
>>>>> <a href="mailto:jdoerfert@anl.gov">jdoerfert@anl.gov</a> <<a href="mailto:jdoerfert@anl.gov">mailto:jdoerfert@anl.gov</a>><br>
> <br>
> <br>
> <br>
> <br>
> -- <br>
> <br>
> Johannes Doerfert<br>
> Researcher<br>
> <br>
> Argonne National Laboratory<br>
> Lemont, IL 60439, USA<br>
> <br>
> <a href="mailto:jdoerfert@anl.gov">jdoerfert@anl.gov</a><br>
</div>
</span></font></div>
</div>
</blockquote>
</body>
</html>