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

[Tom Roeder]

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.

Tom

On Fri, Sep 20, 2013 at 10:17 AM, Hans Wennborg <hans at chromium.org> wrote:
> Hi Tom,
>
> I don't know the SROA code at all, but this seems relevant to my
> interests (PR17271), so I'll do my best to look at it :)

Yes, that looks very similar.

>
>> +  if (NewTy->isVectorTy() || OldTy->isVectorTy()) {
>> +    // The conversion between <n x type*> and any other non-vector type is not possible
>
> I found the comment confusing at this stage because I guess it refers
> to the logic further down (or for this block as a whole) rather than
> to just the next if statement. Also 80 columns.
>
>> +    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. If not, I
should add another test case for this. Do you know of a case where
conversion between two vectors of the same width in bits is not
possible?


>
>> +
>> +    if (VectorType *VecTy = dyn_cast<VectorType>(NewTy)) {
>> +      if (VecTy->getElementType()->isPointerTy())
>> +        return false;
>> +    } else if (VectorType *VecTy = dyn_cast<VectorType>(OldTy)) {
>> +      if (VecTy->getElementType()->isPointerTy())
>> +        return false;
>> +    }
>
> Would it be easier to read as something like the code below?
>
>   // Conversion from vector of ptr to non-vector is not possible.
>   VectorType *VecTy = cast<VectorType>(OldTy->isVectorTy() ? OldTy : NewTy);
>   if (VecTy->getElementType()->isPoitnerTy())
>     return false;


>
> (Also, Benjamin pointed out in
> http://llvm.org/bugs/show_bug.cgi?id=17271#c3 that we could convert
> like <2 x i8*> -> <2 x i32> -> i64, but that could be the subject of
> another patch.)

I'm happy to leave this for another patch. :)

>
>> @@ -1479,9 +1493,16 @@
>>      if (IntegerType *NewITy = dyn_cast<IntegerType>(Ty))
>>        if (NewITy->getBitWidth() > OldITy->getBitWidth())
>>          return IRB.CreateZExt(V, NewITy);
>> -  if (V->getType()->isIntegerTy() && Ty->isPointerTy())
>> +  Type *SrcTy = V->getType();
>> +  Type *DstTy = Ty;
>> +  if (SrcTy->isVectorTy())
>> +    SrcTy = cast<VectorType>(SrcTy)->getElementType();
>> +  if (DstTy->isVectorTy())
>> +    DstTy = cast<VectorType>(DstTy)->getElementType();
>
> Maybe using dyn_cast<> for these would be more common LLVM style, i.e.
> something like:
>
> if (VectorType *SrcVecTy = dyn_cast<VectorType>(SrcTy))
>   SrcTy = SrcVecTy->getElementType();
>
> 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.

>
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sroa_vector.patch
Type: application/octet-stream
Size: 3304 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130920/670dab38/attachment.obj>