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

[David Blaikie via llvm-commits]

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...
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150914/a528b0c3/attachment.html>