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

[Hans Wennborg]

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 :)

> +  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.

> +
> +    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.)

> @@ -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?

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