[llvm] r191122 - LoopVectorizer: Only allow vectorization of intrinsics. We can't know for sure that the functions 'abs' or 'round' are the functions from libm.

[Arnold Schwaighofer]

We were also vectorizing calls to functions with internal linkage (which I believe are allowed). So, I think Nadav was right in disabling it - as replacing calls to functions with internal linkage with intrinsics is a bug (at least in my interpretation of 7.1.3 such function calls would be allowed but IANACLL ;).

I agree, however that we can do better than just giving up. As Hal said, I believe that it is UB if the function has external linkage (or the corresponding header is included). CodeGen makes use of this and replaces function calls but it checks the signature and linkage first.

I suggest we resurrect the patch with those checks in.

Patch attached.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Revert-LoopVectorizer-Only-allow-vectorization-of-in.patch
Type: application/octet-stream
Size: 10203 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130921/eb2904f1/attachment.obj>
-------------- next part --------------



On Sep 20, 2013, at 7:44 PM, Hal Finkel <hfinkel at anl.gov> wrote:

> Nadav,
> 
> This represents a major functionality regression. The entire TLI infrastructure depends on matching functions by name, and this is no different. Are you going to have the frontend turn all of these into intrinsics, and only when math.h in included?
> 
> In any case, all of those names are reserved (when used as identifiers with external linkage), at least in C (according to section 7.1.3 Reserved identifiers). Could the user specify -fno-builtins?
> 
> Thanks again,
> Hal
> 
> ----- Original Message -----
>> Author: nadav
>> Date: Fri Sep 20 19:27:05 2013
>> New Revision: 191122
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=191122&view=rev
>> Log:
>> LoopVectorizer: Only allow vectorization of intrinsics. We can't know
>> for sure that the functions 'abs' or 'round' are the functions from
>> libm.
>> 
>> rdar://15012650
>> 
>> 
>> Modified:
>>    llvm/trunk/lib/Transforms/Vectorize/LoopVectorize.cpp
>>    llvm/trunk/test/Transforms/LoopVectorize/intrinsic.ll
>> 
>> Modified: llvm/trunk/lib/Transforms/Vectorize/LoopVectorize.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Vectorize/LoopVectorize.cpp?rev=191122&r1=191121&r2=191122&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Transforms/Vectorize/LoopVectorize.cpp (original)
>> +++ llvm/trunk/lib/Transforms/Vectorize/LoopVectorize.cpp Fri Sep 20
>> 19:27:05 2013
>> @@ -2925,9 +2925,18 @@ bool LoopVectorizationLegality::canVecto
>>       // We still don't handle functions. However, we can ignore dbg
>>       intrinsic
>>       // calls and we do handle certain intrinsic and libm
>>       functions.
>>       CallInst *CI = dyn_cast<CallInst>(it);
>> -      if (CI && !getIntrinsicIDForCall(CI, TLI) &&
>> !isa<DbgInfoIntrinsic>(CI)) {
>> +      if (CI) {
>>         DEBUG(dbgs() << "LV: Found a call site.\n");
>> -        return false;
>> +
>> +        if (!isa<IntrinsicInst>(it)) {
>> +          DEBUG(dbgs() << "LV: We only vectorize intrinsics.\n");
>> +          return false;
>> +        }
>> +
>> +        if (!getIntrinsicIDForCall(CI, TLI) &&
>> !isa<DbgInfoIntrinsic>(CI)) {
>> +          DEBUG(dbgs() << "LV: Found an unknown intrinsic.\n");
>> +          return false;
>> +        }
>>       }
>> 
>>       // Check that the instruction return type is vectorizable.
>> 
>> Modified: llvm/trunk/test/Transforms/LoopVectorize/intrinsic.ll
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/LoopVectorize/intrinsic.ll?rev=191122&r1=191121&r2=191122&view=diff
>> ==============================================================================
>> --- llvm/trunk/test/Transforms/LoopVectorize/intrinsic.ll (original)
>> +++ llvm/trunk/test/Transforms/LoopVectorize/intrinsic.ll Fri Sep 20
>> 19:27:05 2013
>> @@ -1018,7 +1018,7 @@ for.body:
>>   %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %for.body
>>   ]
>>   %arrayidx = getelementptr inbounds float* %x, i64 %indvars.iv
>>   %0 = load float* %arrayidx, align 4
>> -  %call = tail call float @fabsf(float %0) nounwind readnone
>> +  %call = tail call float @llvm.fabs.f32(float %0) nounwind readnone
>>   store float %call, float* %arrayidx, align 4
>>   %indvars.iv.next = add i64 %indvars.iv, 1
>>   %lftr.wideiv = trunc i64 %indvars.iv.next to i32
>> @@ -1029,6 +1029,31 @@ for.end:
>>   ret void
>> }
>> 
>> -declare float @fabsf(float) nounwind readnone
>> -
>> declare double @llvm.pow.f64(double, double) nounwind readnone
>> +
>> +
>> +;CHECK: @not_intrin
>> +;CHECK: @round
>> +;CHECK-NOT: @round
>> +;CHECK: ret
>> +define void @not_intrin(i32* nocapture %A) nounwind ssp uwtable {
>> +  br label %1
>> +
>> +; <label>:1                                       ; preds = %1, %0
>> +  %indvars.iv = phi i64 [ 0, %0 ], [ %indvars.iv.next, %1 ]
>> +  %2 = getelementptr inbounds i32* %A, i64 %indvars.iv
>> +  %3 = load i32* %2, align 4
>> +  %4 = add nsw i32 %3, 3
>> +  store i32 %4, i32* %2, align 4
>> +  %5 = trunc i64 %indvars.iv to i32
>> +  tail call void @round(i32 %5) nounwind
>> +  %indvars.iv.next = add i64 %indvars.iv, 1
>> +  %lftr.wideiv = trunc i64 %indvars.iv.next to i32
>> +  %exitcond = icmp eq i32 %lftr.wideiv, 256
>> +  br i1 %exitcond, label %6, label %1
>> +
>> +; <label>:6                                       ; preds = %1
>> +  ret void
>> +}
>> +
>> +declare void @round(i32)
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
> 
> -- 
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits