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

[JF Bastien via llvm-commits]

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