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

[JF Bastien via llvm-commits]

On Mon, Sep 14, 2015 at 2:21 PM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Mon, Sep 14, 2015 at 2:17 PM, JF Bastien <jfb at google.com> wrote:
>
>> On Mon, Sep 14, 2015 at 2:12 PM, David Blaikie <dblaikie at gmail.com>
>> wrote:
>>
>>>
>>>
>>> On Mon, Sep 14, 2015 at 2:09 PM, JF Bastien <jfb at google.com> wrote:
>>>
>>>> On Mon, Sep 14, 2015 at 1:25 PM, David Blaikie <dblaikie at gmail.com>
>>>> wrote:
>>>>
>>>>>
>>>>>
>>>>> 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.
>>>>>
>>>>
>>>> I think so? I'm not sure I follow anymore :-)
>>>> This pass wants to find functions that have exactly the same effect, so
>>>> if the stores end up putting the same bytes at the same place in the same
>>>> order then the functions are equivalent and can be merged.
>>>>
>>>
>>> Those two functions don't (one stores 4 words, the other stores 2) - I'm
>>> wondering what part of MergeFunc would correctly diagnose f1 and f2 as not
>>> equivalent. Also, if there's some other way to structure the code such that
>>> the gep's vector type wouldn't be observed by some other thing (eg: I
>>> imagine the store equivalence testing would do value equivelence testing
>>> and observe that {1, 2} != {1, 2, 3, 4}, so even though the geps look
>>> identical to the comparison, the equivalence test would still safely fail)
>>>
>>
>> Wouldn't the getSourceElementType test fail?
>>
>
> Sorry, no - the source element type is the pointee type - in a vector gep
> the 'pointer' operand is actually a vector of pointers. So the source
> element type is the pointee type of those pointers inside the vector.
>
> pointer operand type: [4 x i32*]
> source element type: i32
>
> pointer operand type: i32*
> source element type: i32
>
> etc...
>
>

Ah I see: in that case the consumer of the GEP would be different, so the
stores would fail to compare and prevent merging.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150914/6b542fe7/attachment.html>