[VPLAN] Minor improvement to testing and debug messages.

[fpetrogalli]

Summary

1. Use computed VF for stress testing.

2. If the computed VF does not produce vector code (VF smaller than 2), force VF to be 4.

3. Test vectorization of i64 data on AArch64 to make sure we generate VF != 4 (on X86 that was already tested on AVX).

[fhahn]

Thanks Francesco. I think we can only remove the arbitrary picking of VF = 4 with VPlanStressTest. I think we still need the other parts for testing, to build VPlans for loops without the pragma, which are then not vectorized.

lib/Transforms/Vectorize/LoopVectorize.cpp

267 ↗

(On Diff #192692)

Ok I think we still want to keep this one here, as it enables building VPlans for loops without the pragma. But we can remove the code to arbitrarily pick VF = 4.

6116 ↗

(On Diff #192692)

We still need this for correctness

[hsaito]

lib/Transforms/Vectorize/LoopVectorize.cpp

6106 ↗

(On Diff #192692)

If we are removing stress testing, I'd like to make sure that, there is a mode where we always get VF > 1. Vectorizing for AVX2 almost fits the profile (double complex is 128bit, YMM register is 256bit), but at Intel, we don't want to stop at just thinking about POD types.

If that is not the case, there is a value in stress testing, at least until we become confident that VPlan vectorizer can handle all cases of OpenMP simd and declare simd. Helps our development.

[hsaito]

lib/Transforms/Vectorize/LoopVectorize.cpp

267 ↗

(On Diff #192692)

We need VF>1 to make this meaningful, and I don't think we'll get that from the current VF determination code. I'm fine setting VF=4 only when the computed VF is 1. Blindly setting VF to a fixed value gives more determinism, though.

[sguggill]

lib/Transforms/Vectorize/LoopVectorize.cpp

1404 ↗

(On Diff #192692)

The intent here is to stress test VPlan build for any outer loop that is reducible (even ones that are not marked with clang vectorize enable). We lose that with this change and I don't think that is the right thing. Given this I think, we will need the VPlanBuildStressTest flag.

[fpetrogalli]

Hi all,

by the comment so far, I think we can abandon this change-set, because:

1. As all of you have pointed out, we still need to do stress testing.
2. Forcing VF=4 for stress testing seems to be a better choice than forcing VF=4 only when the computed VF =1, as it seems more reliable (stress test output doesn't change if we update the bits that compute the VF.

If you agree, I will abandon this patch.

Francesco

[dcaballe]

Yep, I agree on that we should keep the stress testing mechanism. It will be very useful to make sure that the construction and predication (and maybe other transformation) are robust enough since we can run it on loop nests that are not necessarily vectorizable.
Something important, though, is that we shouldn't use this mechanism to bypass legality or pragma simd requirements to vectorize a loop, i.e., we shouldn't use it to generate actual vector code.
What are you trying to achieve, Francesco?

[fhahn]

In D59952#1446670, @dcaballe wrote:

Yep, I agree on that we should keep the stress testing mechanism. It will be very useful to make sure that the construction and predication (and maybe other transformation) are robust enough since we can run it on loop nests that are not necessarily vectorizable.
Something important, though, is that we shouldn't use this mechanism to bypass legality or pragma simd requirements to vectorize a loop, i.e., we shouldn't use it to generate actual vector code.
What are you trying to achieve, Francesco?

I suggested removing setting VF = 4 for VPlanStressTest now that we programmatically determine the VF, to streamline things a bit. In a way, just have VPlanStressTest mean: build a VPlan for any loop you can, with the automatically chosen VF.

I thought the point was to just have a VF to build a VPlan with and as it currently stands we build the same VPlans with VF = 4 or any other VF I think. With the programmatically determined VF, we would be a little closer to reality in the stress testing. As Hideki mentioned, having a constant factor for stress testing might have other benefits later on though

[fpetrogalli]

In D59952#1446685, @fhahn wrote:

In D59952#1446670, @dcaballe wrote:

Yep, I agree on that we should keep the stress testing mechanism. It will be very useful to make sure that the construction and predication (and maybe other transformation) are robust enough since we can run it on loop nests that are not necessarily vectorizable.
Something important, though, is that we shouldn't use this mechanism to bypass legality or pragma simd requirements to vectorize a loop, i.e., we shouldn't use it to generate actual vector code.
What are you trying to achieve, Francesco?

I suggested removing setting VF = 4 for VPlanStressTest now that we programmatically determine the VF, to streamline things a bit. In a way, just have VPlanStressTest mean: build a VPlan for any loop you can, with the automatically chosen VF.

yeah, this patch was a follow up on the comment from @fhahn : https://reviews.llvm.org/D57598#1444238

I might have misunderstood his request. I will abandon the patch.

Thank you,

Francesco

[dcaballe]

I thought the point was to just have a VF to build a VPlan with and as it currently stands we build the same VPlans with VF = 4 or any other VF I think. With the programmatically determined VF, we would be a little closer to reality in the stress testing. As Hideki mentioned, having a constant factor for stress testing might have other benefits later on though

Yes, that sounds good to me. In the worst case, we could always use stress testing + user VF to achieve that.

[fpetrogalli]

I have updated the patch to address all the concerns. Now VF = 4 for stress testing only if the computed VF does not produce vector code (VF < 2).

Thank you all.

[hsaito]

LGTM

[dcaballe]

Should we add a small lit test that covers the new expected behavior? Probably reusing one from D57598 with the stress testing flag would suffice.

[fpetrogalli]

In D59952#1447146, @dcaballe wrote:

Should we add a small lit test that covers the new expected behavior? Probably reusing one from D57598 with the stress testing flag would suffice.

Good idea. Something that invokes stress testing and sets VF = 4 when the computed VF is 1. Did I get it right?

[dcaballe]

In D59952#1447695, @fpetrogalli wrote:

In D59952#1447146, @dcaballe wrote:

Should we add a small lit test that covers the new expected behavior? Probably reusing one from D57598 with the stress testing flag would suffice.

Good idea. Something that invokes stress testing and sets VF = 4 when the computed VF is 1. Did I get it right?

For example. Or one where VF != 4 (computed by determineVPlanVF).

[fpetrogalli]

In D59952#1447953, @dcaballe wrote:

For example. Or one where VF != 4 (computed by determineVPlanVF).

I believe that the test in test/Transforms/LoopVectorize/X86/outer_loop_test1_no_explicit_vect_width.ll is already doing that, as the AVX output is producing code with VF = 8.

Or are you saying this needs to be done explicitly for stress testing?

[fpetrogalli]

I have addressed all concern raised in this review.

In particular, I have added a test for computed VF != 4 on AArch64 (on X86 that was just the case for AVX).

[dcaballe]

Or are you saying this needs to be done explicitly for stress testing?

Yes, something that covers the new stress testing behavior that this patch is introducing, to make sure that we preserve it. It could be a test using the stress testing flag and VF!=4 coming from determinePlanVF and/or a test using stress testing flag and where the default VF=4 is taken.
We are not generating code in stress testing mode so maybe you can match some VF information from the debug print.
Please, let me know if you find any roadblock.

[fpetrogalli]

@dcaballe , gentle ping.

[fhahn]

Thanks Francesco! I think the test looks good now. If you need someone to commit it, just let me know.

[dcaballe]

LGTM! Thanks, Francesco!

[fpetrogalli]

Thank you all.

@fhahn , did you say you volunteered for committing the patch? Please proceed. :)

Francesco

[dstenb]

Hi! One of the tests was missing "REQUIRES: asserts", so it failed on a non-assert build. I took the liberty to add that to the test in r358057. I hope that was okay!

[fpetrogalli]

In D59952#1460868, @dstenb wrote:

Hi! One of the tests was missing "REQUIRES: asserts", so it failed on a non-assert build. I took the liberty to add that to the test in r358057. I hope that was okay!

Hi - thank your for taking care of this, apologies for breaking your runs!

Do you need a review?

Francesco

llvm/trunk/lib/Transforms/Vectorize/LoopVectorize.cpp

LoopVectorizationPlanner::planInVPlanNativePath(bool OptForSize,
   // Outer loop handling: They may require CFG and instruction level
   // transformations before even evaluating whether vectorization is profitable.
   // Since we cannot modify the incoming IR, we need to build VPlan upfront in
   // the vectorization pipeline.
   if (!OrigLoop->empty()) {
     // If the user doesn't provide a vectorization factor, determine a
     // reasonable one.
     if (!UserVF) {
-      // We set VF to 4 for stress testing.
-      if (VPlanBuildStressTest)
-        VF = 4;
-      else
-        VF = determineVPlanVF(TTI->getRegisterBitWidth(true /* Vector*/), CM);
-    }
+      LLVM_DEBUG(dbgs() << "LV: VPlan computed VF " << VF << ".\n");
 
+      // Make sure we have a VF > 1 for stress testing.
+      if (VPlanBuildStressTest && VF < 2) {
+        LLVM_DEBUG(dbgs() << "LV: VPlan stress testing: "
+                          << "overriding computed VF.\n");
+        VF = 4;
+      }
+    }
     assert(EnableVPlanNativePath && "VPlan-native path is not enabled.");
     assert(isPowerOf2_32(VF) && "VF needs to be a power of two");
-    LLVM_DEBUG(dbgs() << "LV: Using " << (UserVF ? "user VF " : "computed VF ")
-                      << VF << " to build VPlans.\n");
+    LLVM_DEBUG(dbgs() << "LV: Using " << (UserVF ? "user " : "") << "VF " << VF
+                      << " to build VPlans.\n");
     buildVPlans(VF, VF);
 
     // For VPlan build stress testing, we bail out after VPlan construction.
     if (VPlanBuildStressTest)
       return VectorizationFactor::Disabled();
 
     return {VF, 0};
   }

llvm/trunk/test/Transforms/LoopVectorize/AArch64/outer_loop_test1_no_explicit_vect_width.ll

 ;   for (i1 = 0; i1 < 8; i1++) {
 ;     arr2[i1] = i1;
 ;     for (i2 = 0; i2 < 8; i2++)
 ;       arr[i2][i1] = i1 + n;
 ;   }
 ; }
 ;
 
+; CHECK-LABEL: @foo_i32(
 ; CHECK-LABEL: vector.ph:
 ; CHECK: %[[SplatVal:.*]] = insertelement <4 x i32> undef, i32 %n, i32 0
 ; CHECK: %[[Splat:.*]] = shufflevector <4 x i32> %[[SplatVal]], <4 x i32> undef, <4 x i32> zeroinitializer
 
 ; CHECK-LABEL: vector.body:
 ; CHECK: %[[Ind:.*]] = phi i64 [ 0, %vector.ph ], [ %[[IndNext:.*]], %[[ForInc:.*]] ]
 ; CHECK: %[[VecInd:.*]] = phi <4 x i64> [ <i64 0, i64 1, i64 2, i64 3>, %vector.ph ], [ %[[VecIndNext:.*]], %[[ForInc]] ]
 ; CHECK: %[[AAddr:.*]] = getelementptr inbounds [8 x i32], [8 x i32]* @arr2, i64 0, <4 x i64> %[[VecInd]]

 ; CHECK: %[[IndNext]] = add i64 %[[Ind]], 4
 ; CHECK: %[[VecIndNext]] = add <4 x i64> %[[VecInd]], <i64 4, i64 4, i64 4, i64 4>
 ; CHECK: %[[Cmp:.*]] = icmp eq i64 %[[IndNext]], 8
 ; CHECK: br i1 %[[Cmp]], label %middle.block, label %vector.body
 
 @arr2 = external global [8 x i32], align 16
 @arr = external global [8 x [8 x i32]], align 16
 
+@arrX = external global [8 x i64], align 16
+@arrY = external global [8 x [8 x i64]], align 16
+
 ; Function Attrs: norecurse nounwind uwtable
-define void @foo(i32 %n) {
+define void @foo_i32(i32 %n) {
 entry:
   br label %for.body
 
 for.body:                                         ; preds = %for.inc8, %entry
   %indvars.iv21 = phi i64 [ 0, %entry ], [ %indvars.iv.next22, %for.inc8 ]
   %arrayidx = getelementptr inbounds [8 x i32], [8 x i32]* @arr2, i64 0, i64 %indvars.iv21
   %0 = trunc i64 %indvars.iv21 to i32
   store i32 %0, i32* %arrayidx, align 4
for.inc8:                                         ; preds = %for.body3
   %indvars.iv.next22 = add nuw nsw i64 %indvars.iv21, 1
   %exitcond23 = icmp eq i64 %indvars.iv.next22, 8
   br i1 %exitcond23, label %for.end10, label %for.body, !llvm.loop !1
 
 for.end10:                                        ; preds = %for.inc8
   ret void
 }
 
+; CHECK-LABEL: @foo_i64(
+; CHECK-LABEL: vector.ph:
+; CHECK: %[[SplatVal:.*]] = insertelement <2 x i64> undef, i64 %n, i32 0
+; CHECK: %[[Splat:.*]] = shufflevector <2 x i64> %[[SplatVal]], <2 x i64> undef, <2 x i32> zeroinitializer
+
+; CHECK-LABEL: vector.body:
+; CHECK: %[[Ind:.*]] = phi i64 [ 0, %vector.ph ], [ %[[IndNext:.*]], %[[ForInc:.*]] ]
+; CHECK: %[[VecInd:.*]] = phi <2 x i64> [ <i64 0, i64 1>, %vector.ph ], [ %[[VecIndNext:.*]], %[[ForInc]] ]
+; CHECK: %[[AAddr:.*]] = getelementptr inbounds [8 x i64], [8 x i64]* @arrX, i64 0, <2 x i64> %[[VecInd]]
+; CHECK: call void @llvm.masked.scatter.v2i64.v2p0i64(<2 x i64> %[[VecInd]], <2 x i64*> %[[AAddr]], i32 4, <2 x i1> <i1 true, i1 true>)
+; CHECK: %[[StoreVal:.*]] = add nsw <2 x i64> %[[VecInd]], %[[Splat]]
+; CHECK: br label %[[InnerLoop:.+]]
+
+; CHECK: [[InnerLoop]]:
+; CHECK: %[[InnerPhi:.*]] = phi <2 x i64> [ %[[InnerPhiNext:.*]], %[[InnerLoop]] ], [ zeroinitializer, %vector.body ]
+; CHECK: %[[AAddr2:.*]] = getelementptr inbounds [8 x [8 x i64]], [8 x [8 x i64]]* @arrY, i64 0, <2 x i64> %[[InnerPhi]], <2 x i64> %[[VecInd]]
+; CHECK: call void @llvm.masked.scatter.v2i64.v2p0i64(<2 x i64> %[[StoreVal]], <2 x i64*> %[[AAddr2]], i32 4, <2 x i1> <i1 true, i1 true>
+; CHECK: %[[InnerPhiNext]] = add nuw nsw <2 x i64> %[[InnerPhi]], <i64 1, i64 1>
+; CHECK: %[[VecCond:.*]] = icmp eq <2 x i64> %[[InnerPhiNext]], <i64 8, i64 8>
+; CHECK: %[[InnerCond:.*]] = extractelement <2 x i1> %[[VecCond]], i32 0
+; CHECK: br i1 %[[InnerCond]], label %[[ForInc]], label %[[InnerLoop]]
+
+; CHECK: [[ForInc]]:
+; CHECK: %[[IndNext]] = add i64 %[[Ind]], 2
+; CHECK: %[[VecIndNext]] = add <2 x i64> %[[VecInd]], <i64 2, i64 2>
+; CHECK: %[[Cmp:.*]] = icmp eq i64 %[[IndNext]], 8
+; CHECK: br i1 %[[Cmp]], label %middle.block, label %vector.body
+; Function Attrs: norecurse nounwind uwtable
+define void @foo_i64(i64 %n) {
+entry:
+  br label %for.body
+
+for.body:                                         ; preds = %for.inc8, %entry
+  %indvars.iv21 = phi i64 [ 0, %entry ], [ %indvars.iv.next22, %for.inc8 ]
+  %arrayidx = getelementptr inbounds [8 x i64], [8 x i64]* @arrX, i64 0, i64 %indvars.iv21
+  store i64 %indvars.iv21, i64* %arrayidx, align 4
+  %add = add nsw i64 %indvars.iv21, %n
+  br label %for.body3
+
+for.body3:                                        ; preds = %for.body3, %for.body
+  %indvars.iv = phi i64 [ 0, %for.body ], [ %indvars.iv.next, %for.body3 ]
+  %arrayidx7 = getelementptr inbounds [8 x [8 x i64]], [8 x [8 x i64]]* @arrY, i64 0, i64 %indvars.iv, i64 %indvars.iv21
+  store i64 %add, i64* %arrayidx7, align 4
+  %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1
+  %exitcond = icmp eq i64 %indvars.iv.next, 8
+  br i1 %exitcond, label %for.inc8, label %for.body3
+
+for.inc8:                                         ; preds = %for.body3
+  %indvars.iv.next22 = add nuw nsw i64 %indvars.iv21, 1
+  %exitcond23 = icmp eq i64 %indvars.iv.next22, 8
+  br i1 %exitcond23, label %for.end10, label %for.body, !llvm.loop !1
+
+for.end10:                                        ; preds = %for.inc8
+  ret void
+}
+
+
 !1 = distinct !{!1, !2}
 !2 = !{!"llvm.loop.vectorize.enable", i1 true}

llvm/trunk/test/Transforms/LoopVectorize/explicit_outer_detection.ll

for.end15:                                        ; preds = %outer.inc, %entry
   ret void
 }
 
 ; Case 2: Annotated outer loop WITHOUT vector width information must be collected.
 
 ; CHECK-LABEL: case2
 ; CHECK: LV: Loop hints: force=enabled width=0 unroll=0
 ; CHECK: LV: We can vectorize this outer loop!
-; CHECK: LV: Using computed VF 1 to build VPlans.
+; CHECK: LV: Using VF 1 to build VPlans.
 
 define void @case2(i32* nocapture %a, i32* nocapture readonly %b, i32 %N, i32 %M) local_unnamed_addr {
 entry:
   %cmp32 = icmp sgt i32 %N, 0
   br i1 %cmp32, label %outer.ph, label %for.end15
 
 outer.ph:                                          ; preds = %entry
   %cmp230 = icmp sgt i32 %M, 0

llvm/trunk/test/Transforms/LoopVectorize/vplan-stress-test-no-explict-vf.ll

+; RUN: opt < %s  -S -loop-vectorize -enable-vplan-native-path -vplan-build-stress-test -debug-only=loop-vectorize -disable-output 2>&1  | FileCheck %s
+
+; This test checks that, when stress testing VPlan, if the computed VF
+; is 1, we override it to VF = 4.
+
+; CHECK: LV: VPlan computed VF 1.
+; CHECK: LV: VPlan stress testing: overriding computed VF.
+; CHECK: LV: Using VF 4 to build VPlans.
+@arr2 = external global [8 x i32], align 16
+@arr = external global [8 x [8 x i32]], align 16
+
+; Function Attrs: norecurse nounwind uwtable
+define void @foo(i32 %n) {
+entry:
+  br label %for.body
+
+for.body:                                         ; preds = %for.inc8, %entry
+  %indvars.iv21 = phi i64 [ 0, %entry ], [ %indvars.iv.next22, %for.inc8 ]
+  %arrayidx = getelementptr inbounds [8 x i32], [8 x i32]* @arr2, i64 0, i64 %indvars.iv21
+  %0 = trunc i64 %indvars.iv21 to i32
+  store i32 %0, i32* %arrayidx, align 4
+  %1 = trunc i64 %indvars.iv21 to i32
+  %add = add nsw i32 %1, %n
+  br label %for.body3
+
+for.body3:                                        ; preds = %for.body3, %for.body
+  %indvars.iv = phi i64 [ 0, %for.body ], [ %indvars.iv.next, %for.body3 ]
+  %arrayidx7 = getelementptr inbounds [8 x [8 x i32]], [8 x [8 x i32]]* @arr, i64 0, i64 %indvars.iv, i64 %indvars.iv21
+  store i32 %add, i32* %arrayidx7, align 4
+  %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1
+  %exitcond = icmp eq i64 %indvars.iv.next, 8
+  br i1 %exitcond, label %for.inc8, label %for.body3
+
+for.inc8:                                         ; preds = %for.body3
+  %indvars.iv.next22 = add nuw nsw i64 %indvars.iv21, 1
+  %exitcond23 = icmp eq i64 %indvars.iv.next22, 8
+  br i1 %exitcond23, label %for.end10, label %for.body, !llvm.loop !1
+
+for.end10:                                        ; preds = %for.inc8
+  ret void
+}
+
+!1 = distinct !{!1, !2}
+!2 = !{!"llvm.loop.vectorize.enable", i1 true}