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

[David Blaikie via llvm-commits]

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?

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.


>     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/d4bc8149/attachment.html>