<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Sep 8, 2015 at 1:42 PM, Nick Lewycky <span dir="ltr"><<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On 8 September 2015 at 12:41, Piotr Padlewski <span dir="ltr"><<a href="mailto:prazek@google.com" target="_blank">prazek@google.com</a>></span> wrote:<br><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 dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Tue, Sep 8, 2015 at 11:32 AM, Nick Lewycky <span dir="ltr"><<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>></span> wrote:<br><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 dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span>On 8 September 2015 at 11:28, Piotr Padlewski <span dir="ltr"><<a href="mailto:prazek@google.com" target="_blank">prazek@google.com</a>></span> wrote:<br><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">Prazek marked an inline comment as done.<br>
<span><br>
================<br>
Comment at: lib/CodeGen/CodeGenPrepare.cpp:1414-1417<br>
@@ -1413,2 +1413,6 @@<br>
}<br>
+ case Intrinsic::invariant_group_barrier:<br>
+ II->replaceAllUsesWith(II->getArgOperand(0));<br>
+ II->eraseFromParent();<br>
+ return true;<br>
}<br>
----------------<br>
</span><span>nlewycky wrote:<br>
> Do we also discard the !invariant.group metadata on instructions?<br>
><br>
> The backend has pointers to the Value*'s, it would be bad if they used the !invariant.group markers but all the invariant.group.barrier calls had been removed.<br>
</span>The question is, does it make any difference? What I understand, is that CodeGenPrepare is done just before machine code generation, which means that additional metadata in load/stores doesn't make any difference.<br></blockquote><div><br></div></span><div>Today it does not. My point is that machine code generation does look at Value*. It does things like alias analysis on the IR. Leaving the !invariant.group there is a trap for a future developer.</div></div></div></div></blockquote><div><br></div></span><div>Ok, I will get rid of it, but it is not part of this review. <a href="http://reviews.llvm.org/D12026" target="_blank">http://reviews.llvm.org/D12026</a></div></div></div></div></blockquote><div><br></div></span><div>OK.</div><span class=""><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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><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 dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><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"><span>================<br>
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:2507-2508<br>
@@ -2506,1 +2506,4 @@<br>
continue;<br>
+ } else if (II->getIntrinsicID() == Intrinsic::invariant_group_barrier) {<br>
+ setVal(II, getVal(II->getOperand(0)));<br>
+ DEBUG(dbgs() << "Passing through invariant.group.barrier intrinsic.\n");<br>
----------------<br>
</span><span>nlewycky wrote:<br>
> Why is this safe? We have<br>
> %p2 = call @invariant.group.barrier(%p1)<br>
> and we set that %p2 just plain *is* %p1. Then nothing happens until evaluation completes and we commit the changes, which means replacing %p2 with %p1. At the time, why won't that operation miscompile?<br>
><br>
> I can think of two possible reasons, one is that %p1 may be constrained (like, it may be a ConstantExpr) and the other is that we may know that there are no remaining loads of either %p1 or %p2 with !invariant.group on them.<br>
><br>
> In any event, please answer in the form of a comment in this code. :)<br>
</span>What do you mean about miscompile?<br>
I think this is safe because global-opt doesn't care about !invariant.group metadata, which means that getting rid of invariant.group.barrier will not break anything.<br></blockquote><div><br></div></span><div>That's a good start, but why is it impossible to get a case where replacing the pointer is something globalopt does, then the pointer is used by a load in a function that globalopt didn't look at?</div></div></div></div></blockquote></span><div>I am wating for test case as we disscuss </div></div></div></div></blockquote><div><br></div></span><div>@s1 = unnamed_addr internal global i8*<br></div><div>@s2 = unnamed_addr internal global i8*<br></div><div><br></div><div>define void @__global_var_init() {</div><div> %p1 = ...</div><div> %p2 = call i8* @invariant.group.barrier(i8* %p1)</div><div> store i8* %p1, i8** @s1</div><div> store i8* %p2, i8** @s2</div><div> ret void<br>}</div><div><br></div><div>define void @runtime_function() {</div><div> %p1 = load i8*, i8** @s1</div><div> %p2 = load i8*, i8** @s2<br></div><div> %A = load i8* %p1, !invariant.group A</div><div> store ...</div><div> %B = load i8* %p2, !invariant.group A<br></div><div> ...</div><div>}</div><div><br></div><div>The order of optimization is:</div><div> 1. globalopt turns %p2 into %p1 internally</div><div> 2. it puts %p1 into @s1 and @s2</div><div> 3. globalopt evaluation succeeds, it commits %p1 to both @s1 and @s2</div><div> 4. there is no need for @s1 and @s2, they are unnamed_addr and always contain the same value, they are folded into one (@s2->RAUW(@s1))</div><div> 5. therefore %p1 and %p2 are combined (%p2->RAUW(%p1))</div><div> 6. now both %A and %B are loads with the same SSA variable and invariant.group, but they used to have an invariant.group.barrier between them</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>Nick</div><div><br></div></font></span></div></div></div>
</blockquote></div>Ok, but in this case we load the same value so having and not having invariant.group.barrier between them doesn't change anything. I could be wrong if load of %A and %B would be different, but we can't get case like this.</div><div class="gmail_extra">Second thing, is that I don't think we would be able to perform non local optimizations - if __global_bar_init() would be a normal function, then when we would ask about memory dependencie of %B or %A, we would get clobber,</div><div class="gmail_extra">because we would run on the beginning of function. Am I right?</div></div>