[PATCH] Make Scalar Replacement of Aggregates use the right opcode for converting from vector-of-int to vector-of-pointer

[Hans Wennborg]

On Fri, Sep 20, 2013 at 11:55 AM, Tom Roeder <tmroeder at google.com> wrote:
> Thanks for the feedback. Here's an updated patch (rolled forward to
> apply on r191100) and some comments inline. Please let me know what
> you think.


>>
>>> +    if (NewTy->isVectorTy() && OldTy->isVectorTy())
>>> +      return true;
>>
>> Should this call canConvertValue() with the vector types stripped off?
>> Just because they're both vectors I don't think we can always convert.
>
> I've moved the comment down and expanded it somewhat to be more clear.
>
> At this point in canConvertValue(), the code has already checked that
> the two types have the same size in bits, and vectors can only be
> composed of integers, floating point, and pointers, so I believe they
> can always be converted by bitcast, inttoptr, or ptrtoint.

I'm not an expert here, but this sounds plausible :)

>> Also, should the stripping of vector types happen further up, so the
>> conversion with CreateZExt could work with vectors too?
>
> I don't think so: zext only works on integers, according to the
> language reference:
>
> "Zero extend a constant to another type. The bit size of CST must be
> smaller than the bit size of TYPE. Both types must be integers."
>
> This is why convertValue checks first for integers types where one
> type is of greater-than or equal bit width.

I was looking at http://llvm.org/docs/LangRef.html#zext-to-instruction
where it says "Both types must be of integer types, or vectors of the
same number of integers."


Thanks,
Hans

>> On Fri, Sep 20, 2013 at 9:14 AM, Tom Roeder <tmroeder at google.com> wrote:
>>> Ping
>>>
>>> On Fri, Sep 13, 2013 at 4:43 PM, Tom Roeder <tmroeder at google.com> wrote:
>>>> Here's an updated patch with three tests (I found more problems as I
>>>> worked on minimizing my test case) and a check added to
>>>> canConvertValue. Note that AFAICT, I can't just strip the vector type
>>>> then check compatibility of the conversion, since (as one of my tests
>>>> shows) there's a problem that <1 x i32*> can't be converted to i64,
>>>> while the stripped version of the check would try to turn it into a
>>>> ptrtoint.
>>>>
>>>> Please let me know what you think.
>>>>
>>>> Tom
>>>>
>>>> On Fri, Sep 13, 2013 at 1:51 PM, Tom Roeder <tmroeder at google.com> wrote:
>>>>> Actually, I just figured it out. It's probably not minimal yet, but it does
>>>>> trigger the crash:
>>>>>
>>>>> define <4 x i64> @f({<2 x i32*>, <2 x i32*>} %x) {
>>>>>   %a = alloca {<2 x i32*>, <2 x i32*>}
>>>>>   store {<2 x i32*>, <2 x i32*>} %x, {<2 x i32*>, <2 x i32*>}* %a
>>>>>   %cast = bitcast {<2 x i32*>, <2 x i32*>}* %a to <4 x i64>*
>>>>>   %vec = load <4 x i64>* %cast
>>>>>   ret <4 x i64> %vec
>>>>> }
>>>>>
>>>>> On Fri, Sep 13, 2013 at 1:16 PM, Tom Roeder <tmroeder at google.com> wrote:
>>>>>> Sorry, I don't follow your description; I don't know how the SROA
>>>>>> algorithm works on vectors, so I don't know the conditions under which
>>>>>> I can get SROA to convert GEP extraction over vectors to vector loads
>>>>>> and stores. And I haven't yet grokked the code. Under what
>>>>>> circumstances would SROA do these conversions?
>>>>>>
>>>>>> On Thu, Sep 12, 2013 at 3:20 PM, Chandler Carruth <chandlerc at google.com> wrote:
>>>>>>> On Thu, Sep 12, 2013 at 2:54 PM, Tom Roeder <tmroeder at google.com> wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> In the process of trying to compile Chromium with clang -flto -O3,
>>>>>>>> I've run into a crash in SROA;  the pass is trying to cast <2 x i64>
>>>>>>>> to <2 x %"struct.webrtc::WindowCapturer::Window"*> with a bitcast,
>>>>>>>> when it should be using inttoptr. I believe this patch fixes the
>>>>>>>> problem.
>>>>>>>>
>>>>>>>> The problem is that convertValue in lib/Transforms/Scalar/SROA.cpp is
>>>>>>>> being called to convert the int vector into the pointer vector, and it
>>>>>>>> falls through all the cases to the bitcast at the end. So, this
>>>>>>>> patches catches that case and outputs the correct opcode.
>>>>>>>>
>>>>>>>> However, I'm not really familiar with either the code itself or the
>>>>>>>> SROA algorithm, so there might be something wrong with this patch. It
>>>>>>>> fixes my build, and all tests pass in the test suite, but I haven't
>>>>>>>> yet been able to extract a minimal test case from the crash I observe
>>>>>>>> in the massive Chromium LTO build that I've been working on.
>>>>>>>>
>>>>>>>> I'm hoping that this description of the crash and the fix will be
>>>>>>>> enough for someone to point me in the right direction for creating a
>>>>>>>> simple test case that triggers it.
>>>>>>>
>>>>>>>
>>>>>>> You should be able to create a simple test by writing code that extracts
>>>>>>> vector elements with GEPs and getting SROA to convert them to vector loads
>>>>>>> and stores. Then you just want to get one of them to use pointer types
>>>>>>> internally and the others to use integers, forcing the value conversion.
>>>>>>>
>>>>>>> You'll likely need a corresponding change to canConvertValue to support
>>>>>>> vector types.
>>>>>>>
>>>>>>> You should handle this by stripping the vector-ness first, and then handling
>>>>>>> the element type, so that non-vector types and vector element types have the
>>>>>>> same code.
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Tom
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> llvm-commits mailing list
>>>>>>>> llvm-commits at cs.uiuc.edu
>>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>>>>>
>>>>>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits