<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Sep 14, 2015 at 1:12 PM, JF Bastien <span dir="ltr"><<a href="mailto:jfb@google.com" target="_blank">jfb@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"><div class="gmail_extra"><div class="gmail_quote"><span class="">On Mon, Sep 14, 2015 at 1:05 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.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>On Mon, Sep 14, 2015 at 12:59 PM, JF Bastien <span dir="ltr"><<a href="mailto:jfb@google.com" target="_blank">jfb@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"><div class="gmail_extra"><div class="gmail_quote"><span>On Mon, Sep 14, 2015 at 11:25 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.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"><div><div>On Mon, Sep 14, 2015 at 11:13 AM, JF Bastien <span dir="ltr"><<a href="mailto:jfb@google.com" target="_blank">jfb@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><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">GetElementPointers must have the first argument's type compared<br>
for structural equivalence. Previously the code erroneously compared the<br>
pointer's type, but this code was dead because all pointer types (of the<br>
same address space) are the same. The pointee must be compared instead<br>
(using the type stored in the GEP, not from the pointer type which will<br>
be erased anyway).<br></blockquote></span><div><br>Thanks so much for being aware of this/using APIs that'll be forwards compatible with the eventual removal of typed pointers.<br><br>Just in case: do you care about the GEP pointer operands address space (I assume not) or vector types (eg: a gep over a vector of pointers)? I assume not, but just checking - both those aspects of the pointer operand are still in the pointer operand's type, not in the source element type.</div></div></div></div></blockquote><div><br></div></span><div>We do care about address spaces in general: we can't merge two functions if they refer to different address spaces. I'm not sure that's what you're asking for though: in the end we (roughly) only want to merge functions that would codegen to the same code. I think the same applies to your vector of pointers question?</div></div></div></div></blockquote><div><br></div></div></div><div>My point was - do you care if these two GEPs refer to different pointers from address spaces? What if the GEPs are differently vectored (if vector GEPs can exist in this codepath, etc)?<br></div></div></div></div></blockquote><div><br></div></span><div>Yes, we care if the address spaces are different. That's checked at the start of the function.</div><div><br></div><div>By vectored you mean the indexing? I think this is handled properly, but it would be great if you could confirm.</div></div></div></div></blockquote><div><br></div></span><div>I really don't know enough about this pass or code to know if this is even reproducible. Can you get vector geps in this codepath? <br></div></div></div></div></blockquote><div><br></div></span><div>Yes, that should be possible, but IIUC the code compares the effective byte offsets so everything should "just work", unless I'm missing your concern!</div></div></div></div></blockquote><div><br></div><div>So if I'm understanding correctly, this FunctionComparator is trying to test if two functions are equivalent. So I guess the vector-ness of geps would then relate to a test case like this (going to be really pseudo-code-y because I don't know LLVM IR well enough to write it off-the-cuff):<br><br>@blob = global /* big global of data, don't care what it is */<br>void f1() {<br> store getelementptr (i32, bitcast (@blob to [2 x i32*]), ... ), {1, 2}</div><div>}<br>void f2() {<br> store getelementptr (i32, bitcast (@blob to [4 x i32*]), ...), {1, 2, 3, 4}<br>}<br><br>I suppose the store wouldn't match, but I'm not sure - maybe there's a way everything else in the IR could be the same, but the vector-ness of the geps could be differently sized somehow.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class=""><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>getelementptr (i32, [5 x i32*] @x, ... )<br><br><br></div><span><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><div><br></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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>If you do care about the (in)equality of address space or vector-ness in these two GEPs, you'll need to compare those - those would've previously been caught by just comparing the two GEPs pointer operand types, but won't be any more with this change.</div></div></div></div></blockquote><div><br></div><div><br></div></span></div>For reference, the current code is:</div><blockquote style="margin:0 0 0 40px;border:none;padding:0px"><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace">int FunctionComparator::cmpGEPs(const GEPOperator *GEPL,</font></div></div><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace"> const GEPOperator *GEPR) {</font></div></div><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace"><br></font></div></div><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace"> unsigned int ASL = GEPL->getPointerAddressSpace();</font></div></div><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace"> unsigned int ASR = GEPR->getPointerAddressSpace();</font></div></div><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace"><br></font></div></div><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace"> if (int Res = cmpNumbers(ASL, ASR))</font></div></div></blockquote></div></blockquote></span><div><br>Ah, thanks - sorry, hadn't looked at the actual code.<br></div></div></div></div></blockquote><div><br></div></span><div>No worries at all! I'd rather double check that this is correct, versus maybe getting it work. This pass is finicky in that it needs to know *everything* about IR, and it'll go out of date when new properties are added! Jason will propose turning it on by default soon, as far as we know it's now Bug Free (TM) in that all of Chrome compiles and and the tests pass.</div><span class=""><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> <span style="font-family:monospace,monospace"> return Res;</span></div><span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><blockquote style="margin:0 0 0 40px;border:none;padding:0px"><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace"><br></font></div></div><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace"> // When we have target data, we can reduce the GEP down to the value in bytes</font></div></div><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace"> // added to the address.</font></div></div><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace"> const DataLayout &DL = FnL->getParent()->getDataLayout();</font></div></div><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace"> unsigned BitWidth = DL.getPointerSizeInBits(ASL);</font></div></div><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace"> APInt OffsetL(BitWidth, 0), OffsetR(BitWidth, 0);</font></div></div><span><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace"> if (GEPL->accumulateConstantOffset(DL, OffsetL) &&</font></div></div><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace"> GEPR->accumulateConstantOffset(DL, OffsetR))</font></div></div><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace"> return cmpAPInts(OffsetL, OffsetR);</font></div></div></span><span><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace"> if (int Res = cmpTypes(GEPL->getSourceElementType(),</font></div></div></span><span><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace"> GEPR->getSourceElementType()))</font></div></div><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace"> return Res;</font></div></div><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace"><br></font></div></div><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace"> if (int Res = cmpNumbers(GEPL->getNumOperands(), GEPR->getNumOperands()))</font></div></div></span><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace"> return Res;</font></div></div><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace"><br></font></div></div><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace"> for (unsigned i = 0, e = GEPL->getNumOperands(); i != e; ++i) {</font></div></div><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace"> if (int Res = cmpValues(GEPL->getOperand(i), GEPR->getOperand(i)))</font></div></div><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace"> return Res;</font></div></div><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace"> }</font></div></div><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace"><br></font></div></div><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace"> return 0;</font></div></div><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace">}</font></div></div></blockquote><div class="gmail_extra"><div><br></div></div></div>
</blockquote></span></div><br></div></div>
</blockquote></span></div><br></div></div>
</blockquote></div><br></div></div>