<div dir="ltr">Hi Justin,<div><br></div><div>Would you mind checking the NVPTX part? I got LGTM from Fernando and Diogo for the algorithm. </div><div><br></div><div>Thanks,</div><div>Jingyue<br><div><br><div class="gmail_quote">On Wed, Apr 8, 2015 at 9:35 PM Jingyue Wu <<a href="mailto:jingyue@google.com">jingyue@google.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Thanks for your review, Diogo! As you suggested, the new patch (<a href="http://reviews.llvm.org/D8576" target="_blank">http://reviews.llvm.org/D8576</a>) now considers laneid. <div><br></div><div>Jingyue<br><div><br></div><div><div class="gmail_quote"></div></div></div></div><div dir="ltr"><div><div><div class="gmail_quote">On Wed, Apr 8, 2015 at 3:31 PM Diogo Sampaio <<a href="mailto:dnsampaio@gmail.com" target="_blank">dnsampaio@gmail.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Jingyue,<br>
<br>
Very nice code and good comments. It's nice to see this analysis in<br>
llvm and I believe your code is correct. There is one slight point,<br>
not very common, which I faced. The use the intrinsic laneId, most of<br>
the times generated by nvcc, also leads to divergence. I would change<br>
the file NVPTXTargetTransformInfo.cpp either by adding<br>
Intrinsic::int_ptx_read_laneid to the switch in "static bool<br>
readsThreadIndex(const IntrinsicInst *II)" or just creating a method<br>
for it such as...<br>
<br>
static bool readsLaneId(const IntrinsicInst *II) {<br>
return (II->getIntrinsicID() == Intrinsic::int_ptx_read_<u></u>laneid<u></u>);<br>
}<br>
..... and changing the test to ...<br>
if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(I)) {<br>
// Instructions that read threadIdx or laneId are obviously divergent.<br>
if (readsThreadIndex(II) || readsLaneId(II))<br>
return true;<br>
.<br>
<br>
Now, more as an discussion:<br>
In the file DivergenceAnalysis.cpp, line 206<br>
if (!cast<PHINode>(I)-><u></u>hasConstan<u></u>tValue() && DV.insert(I).second)<br>
Worklist.push_back(I);<br>
<br>
I was wondering about the effectiveness of the method PhiNode::hasConstantValue.<br>
For example, in a code such as:<br>
//not a smart code, I know<br>
if(divergentBool){<br>
a0 = b1 - c1;<br>
}else{<br>
c2 = -c1;<br>
a1 = b1 + c2;<br>
}<br>
a2 = phi(a0, a1);<br>
<br></blockquote><div><br></div></div></div></div></div><div dir="ltr"><div><div><div class="gmail_quote"><div>Given the code as is, DivergenceAnalysis will consider a2 divergent. </div><div><br></div><div>However, I wouldn't worry about it. We plan to run this analysis pass at a relatively late stage (definitely after InstCombine), so the code pattern you listed here is likely optimized away. In fact, even a PHINode with same incoming values is probably optimized away. </div></div></div></div></div><div dir="ltr"><div><div><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Will this method be good enough to say that a2 is not divergent? I<br>
believe that llvm's ScalarEvolution could detect they are the same<br>
value.<br>
<br>
And to end, there are 2 typos in your header file:<br>
exeuction<br>
pogram<br>
<br>
<br>
Very nice work,<br>
regards<br>
--<br>
Diogo Nunes Sampaio<br>
<br>
Portable: +33 6 59 05 75 57<br>
<br>
INRIA - Institut national de recherche en informatique et en automatique<br>
CORSE - <a href="http://www.inria.fr/equipes/corse" target="_blank">http://www.inria.fr/equipes/<u></u>co<u></u>rse</a><br>
</blockquote></div></div></div></div></blockquote></div></div></div></div>