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

[Hal Finkel]

Arnold,

Thanks for looking at this quickly.

+static Intrinsic::ID checkBinaryFloatSignature(const CallInst &I,
+                                               Intrinsic::ID ValidIntrinsicID) {
+  if (I.getNumArgOperands() != 2 ||
+      !I.getArgOperand(0)->getType()->isFloatingPointTy() ||
+      !I.getArgOperand(1)->getType()->isFloatingPointTy() ||
+      I.getType() != I.getArgOperand(0)->getType() ||
+      !I.onlyReadsMemory())
+    return Intrinsic::not_intrinsic;
+
+  return ValidIntrinsicID;
+}

To be pedantic, we should check that the two arguments also have the same type (to prevent grabbing copysign(float, double)).

 -Hal

----- Original Message -----
> 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.
> 
> 
> 
> 
> 
> 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
> 
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory