[llvm] r247570 - [MergeFuncs] Fix bug in merging GetElementPointers

[David Blaikie via llvm-commits]

On Mon, Sep 14, 2015 at 1:12 PM, JF Bastien <jfb at google.com> wrote:

> On Mon, Sep 14, 2015 at 1:05 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>
>> On Mon, Sep 14, 2015 at 12:59 PM, JF Bastien <jfb at google.com> wrote:
>>
>>> On Mon, Sep 14, 2015 at 11:25 AM, David Blaikie <dblaikie at gmail.com>
>>> wrote:
>>>
>>>>
>>>>
>>>> On Mon, Sep 14, 2015 at 11:13 AM, JF Bastien <jfb at google.com> wrote:
>>>>
>>>>> GetElementPointers must have the first argument's type compared
>>>>>>> for structural equivalence. Previously the code erroneously compared
>>>>>>> the
>>>>>>> pointer's type, but this code was dead because all pointer types (of
>>>>>>> the
>>>>>>> same address space) are the same. The pointee must be compared
>>>>>>> instead
>>>>>>> (using the type stored in the GEP, not from the pointer type which
>>>>>>> will
>>>>>>> be erased anyway).
>>>>>>>
>>>>>>
>>>>>> Thanks so much for being aware of this/using APIs that'll be forwards
>>>>>> compatible with the eventual removal of typed pointers.
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>
>>>>> 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?
>>>>>
>>>>
>>>> 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)?
>>>>
>>>
>>> Yes, we care if the address spaces are different. That's checked at the
>>> start of the function.
>>>
>>> By vectored you mean the indexing? I think this is handled properly, but
>>> it would be great if you could confirm.
>>>
>>
>> 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?
>>
>
> 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!
>

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):

@blob = global /* big global of data, don't care what it is */
void f1() {
  store getelementptr (i32, bitcast (@blob to [2 x i32*]), ... ), {1, 2}
}
void f2() {
  store getelementptr (i32, bitcast (@blob to [4 x i32*]), ...), {1, 2, 3,
4}
}

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.


>
>
> getelementptr (i32, [5 x i32*] @x, ... )
>>
>>
>>
>>
>>>
>>>
>>> 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.
>>>>
>>>
>>>
>>> For reference, the current code is:
>>>
>>> int FunctionComparator::cmpGEPs(const GEPOperator *GEPL,
>>>                                const GEPOperator *GEPR) {
>>>
>>>   unsigned int ASL = GEPL->getPointerAddressSpace();
>>>   unsigned int ASR = GEPR->getPointerAddressSpace();
>>>
>>>   if (int Res = cmpNumbers(ASL, ASR))
>>>
>>>
>> Ah, thanks - sorry, hadn't looked at the actual code.
>>
>
> 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.
>
>
>      return Res;
>>
>>>
>>>   // When we have target data, we can reduce the GEP down to the value
>>> in bytes
>>>   // added to the address.
>>>   const DataLayout &DL = FnL->getParent()->getDataLayout();
>>>   unsigned BitWidth = DL.getPointerSizeInBits(ASL);
>>>   APInt OffsetL(BitWidth, 0), OffsetR(BitWidth, 0);
>>>   if (GEPL->accumulateConstantOffset(DL, OffsetL) &&
>>>       GEPR->accumulateConstantOffset(DL, OffsetR))
>>>     return cmpAPInts(OffsetL, OffsetR);
>>>   if (int Res = cmpTypes(GEPL->getSourceElementType(),
>>>                          GEPR->getSourceElementType()))
>>>     return Res;
>>>
>>>   if (int Res = cmpNumbers(GEPL->getNumOperands(),
>>> GEPR->getNumOperands()))
>>>     return Res;
>>>
>>>   for (unsigned i = 0, e = GEPL->getNumOperands(); i != e; ++i) {
>>>     if (int Res = cmpValues(GEPL->getOperand(i), GEPR->getOperand(i)))
>>>       return Res;
>>>   }
>>>
>>>   return 0;
>>> }
>>>
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150914/a00b0069/attachment.html>