[Clang] Pragma vectorize_width() implies vectorize(enable), take 3

[SjoerdMeijer]

Summary

This got reverted because given the following source:

void a() {
  #pragma clang loop vectorize(disable)
  for (;;)
    ;
}

it incorrectly enabled vectorisation and set metadata due to a logic error. With this fixed, we now imply vectorisation when:

1. vectorisation is enabled, which means: VectorizeWidth > 1
2. and don't want to add it when it is disabled or enabled, otherwise we would be incorrectly setting it or duplicating the metadata, respectively.

[Meinersbur]

clang/lib/CodeGen/CGLoopInfo.cpp

272–277

[serious] Please handle the llvm.loop.vectorize.enable metadata in one place, i.e. where the other llvm.loop.vectorize.enable is handled. This introduces yet another mechanism when to add llvm.loop.vectorize.enable besides the one for IsVectorPredicateEnabled. Btw, with vectorize_predicate(enable) vectorize_width(2) this emits two llvm.loop.vectorize.enable.

Also, the changing relative order of llvm.loop.vectorize.enable to other metadata makes D69092 difficult.

clang/test/CodeGenCXX/pragma-loop-predicate.cpp

61 ↗

(On Diff #227137)

unintentional change?

clang/test/CodeGenCXX/pragma-loop.cpp

161–177

As you might have noticed, updating FileCheck for metadata in multiple test cases is a lot of work since they influence each other. Why not adding the new test in separate files?

221

-NEXT should be unnecessary here. I'd even go towards CHECK-DAG since the order of the metadata is unimportant.

[SjoerdMeijer]

Thanks Michael!

• moved the handling of vectorize.enable to 1 place,
• that should have also sorted the relative ordering, and duplication of the metadata in some cases,
• created a separate file for the new tests, and added some more tests (also making sure there are no duplicates).

[Meinersbur]

Sorry for the delay, for some reason I did not see a notification for the update.

clang/lib/CodeGen/CGLoopInfo.cpp

297–301

[serious] Why not reusing the Args.push_back code from above? I think it is important vectorize_predicate and vectorize_width (and ever additional property we introduce in the future) the same way. IMHO everything else becomes more and more confusing.
I have the following in mind:

if (Attrs.VectorizeEnable != LoopAttributes::Unspecified ||
      IsVectorPredicateEnabled || Attrs.VectorizeWidth > 1) {
    auto AttrVal = Attrs.VectorizeEnable != LoopAttributes::Disable;
    Args.push_back(..., ConstantInt::get(AttrVal));
}

[SjoerdMeijer]

clang/lib/CodeGen/CGLoopInfo.cpp

297–301

No worries, and thanks for looking again! I was a bit reluctant to touch that piece of logic (and actually thought this if-elseif was not too bad and explicit in identifying the different cases), but yeah, what you suggest make sense, so will address this soon.

[SjoerdMeijer]

Thanks again for the suggestion; this indeed (hopefully) looks a lot neater now.

[Meinersbur]

clang/lib/CodeGen/CGLoopInfo.cpp

297–301

Sounds like a typical case of "technical debt".

[SjoerdMeijer]

clang/lib/CodeGen/CGLoopInfo.cpp

297–301

yep, but it's been addressed

[Meinersbur]

LGTM

clang/lib/CodeGen/CGLoopInfo.cpp

if (Attrs.VectorizePredicateEnable != LoopAttributes::Unspecified &&
     Metadata *Vals[] = {
         MDString::get(Ctx, "llvm.loop.vectorize.predicate.enable"),
         ConstantAsMetadata::get(ConstantInt::get(llvm::Type::getInt1Ty(Ctx),
                                                  IsVectorPredicateEnabled))};
     Args.push_back(MDNode::get(Ctx, Vals));
   }
 
   // Setting vectorize.width
   if (Attrs.VectorizeWidth > 0) {
     Metadata *Vals[] = {
         MDString::get(Ctx, "llvm.loop.vectorize.width"),
         ConstantAsMetadata::get(ConstantInt::get(llvm::Type::getInt32Ty(Ctx),
                                                  Attrs.VectorizeWidth))};
     Args.push_back(MDNode::get(Ctx, Vals));
   }
 
   // Setting interleave.count
   if (Attrs.InterleaveCount > 0) {
     Metadata *Vals[] = {
         MDString::get(Ctx, "llvm.loop.interleave.count"),
         ConstantAsMetadata::get(ConstantInt::get(llvm::Type::getInt32Ty(Ctx),
                                                  Attrs.InterleaveCount))};
     Args.push_back(MDNode::get(Ctx, Vals));
   }
 
-  // Setting vectorize.enable
+  // vectorize.enable is set if:
+  // 1) loop hint vectorize.enable is set, or
+  // 2) it is implied when vectorize.predicate is set, or
+  // 3) it is implied when vectorize.width is set.
   if (Attrs.VectorizeEnable != LoopAttributes::Unspecified ||
-      IsVectorPredicateEnabled) {
-    Metadata *Vals[] = {
-        MDString::get(Ctx, "llvm.loop.vectorize.enable"),
+      IsVectorPredicateEnabled ||
+      Attrs.VectorizeWidth > 1 ) {
+    bool AttrVal = Attrs.VectorizeEnable != LoopAttributes::Disable;
+    Args.push_back(
+        MDNode::get(Ctx, {MDString::get(Ctx, "llvm.loop.vectorize.enable"),
                           ConstantAsMetadata::get(ConstantInt::get(
-            llvm::Type::getInt1Ty(Ctx),
-            IsVectorPredicateEnabled
-                ? true
-                : (Attrs.VectorizeEnable == LoopAttributes::Enable)))};
-    Args.push_back(MDNode::get(Ctx, Vals));
+                              llvm::Type::getInt1Ty(Ctx), AttrVal))}));
   }
 
   if (FollowupHasTransforms)
     Args.push_back(MDNode::get(
         Ctx,
         {MDString::get(Ctx, "llvm.loop.vectorize.followup_all"), Followup}));
 
   MDNode *LoopID = MDNode::get(Ctx, Args);
   LoopID->replaceOperandWith(0, LoopID);

clang/test/CodeGenCXX/pragma-loop-pr27643.cpp

+// RUN: %clang_cc1 -triple x86_64-apple-darwin -std=c++11 -emit-llvm -o - %s | FileCheck %s
+
+void loop1(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}loop1{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP1:.*]]
+
+  #pragma clang loop vectorize(enable) vectorize_width(1)
+  for (int i = 0; i < Length; i++)
+    List[i] = i * 2;
+}
+
+// Here, vectorize.enable should be set, obviously, but also check that
+// metadata isn't added twice.
+void loop2(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}loop2{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP2:.*]]
+
+  #pragma clang loop vectorize(enable) vectorize_width(2)
+  for (int i = 0; i < Length; i++)
+    List[i] = i * 2;
+}
+
+// Test that we do *not* imply vectorize.enable.
+void loop3(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}loop3{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP3:.*]]
+
+  #pragma clang loop vectorize_width(1)
+  for (int i = 0; i < Length; i++)
+    List[i] = i * 2;
+}
+
+// Test that we *do* imply vectorize.enable.
+void loop4(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}loop4{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP4:.*]]
+
+  #pragma clang loop vectorize_width(2)
+  for (int i = 0; i < Length; i++)
+    List[i] = i * 2;
+}
+
+// CHECK: ![[LOOP1]] = distinct !{![[LOOP1]], ![[VEC_WIDTH_1:.*]], ![[VEC_ENABLE:.*]]}
+// CHECK: ![[VEC_WIDTH_1]] = !{!"llvm.loop.vectorize.width", i32 1}
+// CHECK: ![[VEC_ENABLE]] = !{!"llvm.loop.vectorize.enable", i1 true}
+
+// CHECK: ![[LOOP2]] = distinct !{![[LOOP2]], ![[VEC_WIDTH_2:.*]], ![[VEC_ENABLE]]}
+// CHECK: ![[VEC_WIDTH_2]] = !{!"llvm.loop.vectorize.width", i32 2}
+
+// CHECK: ![[LOOP3]] = distinct !{![[LOOP3]], ![[VEC_WIDTH_1]]}
+
+// CHECK: ![[LOOP4]] = distinct !{![[LOOP4]], ![[VEC_WIDTH_2]], ![[VEC_ENABLE]]}

clang/test/CodeGenCXX/pragma-loop.cpp

 void template_test(double *List, int Length) {
   double Value = 10;
 
   for_template_test<double>(List, Length, Value);
   for_template_define_test<double, int>(List, Length, Value);
   for_template_constant_expression_test<double, 2, 4, 8>(List, Length);
 }
 
 // CHECK: ![[LOOP_1]] = distinct !{![[LOOP_1]], ![[UNROLL_FULL:.*]]}
 // CHECK: ![[UNROLL_FULL]] = !{!"llvm.loop.unroll.full"}
 
-// CHECK: ![[LOOP_2]] = distinct !{![[LOOP_2]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_8:.*]], ![[INTERLEAVE_4:.*]]}
+// CHECK: ![[LOOP_2]] = distinct !{![[LOOP_2]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_8:.*]], ![[INTERLEAVE_4:.*]], ![[VECTORIZE_ENABLE:.*]]}
 // CHECK: ![[UNROLL_DISABLE]] = !{!"llvm.loop.unroll.disable"}
 // CHECK: ![[DISTRIBUTE_DISABLE]] = !{!"llvm.loop.distribute.enable", i1 false}
 // CHECK: ![[WIDTH_8]] = !{!"llvm.loop.vectorize.width", i32 8}
 // CHECK: ![[INTERLEAVE_4]] = !{!"llvm.loop.interleave.count", i32 4}
+// CHECK: ![[VECTORIZE_ENABLE]] = !{!"llvm.loop.vectorize.enable", i1 true}
 
-// CHECK: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[INTERLEAVE_4:.*]], ![[INTENABLE_1:.*]], ![[FOLLOWUP_VECTOR_3:.*]]}
-// CHECK: ![[INTENABLE_1]] = !{!"llvm.loop.vectorize.enable", i1 true}
+// CHECK: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[INTERLEAVE_4:.*]], ![[VECTORIZE_ENABLE]], ![[FOLLOWUP_VECTOR_3:.*]]}
 // CHECK: ![[FOLLOWUP_VECTOR_3]] = !{!"llvm.loop.vectorize.followup_all", ![[AFTER_VECTOR_3:.*]]}
 // CHECK: ![[AFTER_VECTOR_3]] = distinct !{![[AFTER_VECTOR_3]], ![[ISVECTORIZED:.*]], ![[UNROLL_8:.*]]}
 // CHECK: ![[ISVECTORIZED]] = !{!"llvm.loop.isvectorized"}
 // CHECK: ![[UNROLL_8]] = !{!"llvm.loop.unroll.count", i32 8}
 
-// CHECK: ![[LOOP_4]] = distinct !{![[LOOP_4]], ![[WIDTH_2:.*]], ![[INTERLEAVE_2:.*]]}
+// CHECK: ![[LOOP_4]] = distinct !{![[LOOP_4]], ![[WIDTH_2:.*]], ![[INTERLEAVE_2:.*]], ![[VECTORIZE_ENABLE]]}
 // CHECK: ![[WIDTH_2]] = !{!"llvm.loop.vectorize.width", i32 2}
 // CHECK: ![[INTERLEAVE_2]] = !{!"llvm.loop.interleave.count", i32 2}
 
 // CHECK: ![[LOOP_5]] = distinct !{![[LOOP_5]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_1:.*]]}
 // CHECK: ![[WIDTH_1]] = !{!"llvm.loop.vectorize.width", i32 1}
 
 // CHECK: ![[LOOP_6]] = distinct !{![[LOOP_6]], ![[WIDTH_2:.*]], ![[INTERLEAVE_2:.*]], ![[FOLLOWUP_VECTOR_6:.*]]}
 // CHECK: ![[FOLLOWUP_VECTOR_6]] = !{!"llvm.loop.vectorize.followup_all", ![[AFTER_VECTOR_6:.*]]}
 // CHECK: ![[AFTER_VECTOR_6]] = distinct !{![[AFTER_VECTOR_6]], ![[ISVECTORIZED:.*]], ![[UNROLL_8:.*]]}
 
-// CHECK: ![[LOOP_7]] = distinct !{![[LOOP_7]], ![[WIDTH_5:.*]]}
+// CHECK: ![[LOOP_7]] = distinct !{![[LOOP_7]], ![[WIDTH_5:.*]], ![[VECTORIZE_ENABLE]]}
 // CHECK: ![[WIDTH_5]] = !{!"llvm.loop.vectorize.width", i32 5}
 
 // CHECK: ![[LOOP_8]] = distinct !{![[LOOP_8]], ![[WIDTH_5:.*]]}
 
 // CHECK: ![[LOOP_9]] = distinct !{![[LOOP_9]], ![[WIDTH_8:.*]], ![[INTERLEAVE_8:.*]], ![[FOLLOWUP_VECTOR_9:.*]]}
 // CHECK: ![[FOLLOWUP_VECTOR_9]] = !{!"llvm.loop.vectorize.followup_all", ![[AFTER_VECTOR_9:.*]]}
 // CHECK: ![[AFTER_VECTOR_9]] = distinct !{![[AFTER_VECTOR_9]], ![[ISVECTORIZED:.*]], ![[UNROLL_8:.*]]}
 
 // CHECK: ![[LOOP_10]] = distinct !{![[LOOP_10]], ![[WIDTH_2:.*]], ![[INTERLEAVE_2:.*]], ![[FOLLOWUP_VECTOR_10:.*]]}
 // CHECK: ![[FOLLOWUP_VECTOR_10]] = !{!"llvm.loop.vectorize.followup_all", ![[AFTER_VECTOR_10:.*]]}
 // CHECK: ![[AFTER_VECTOR_10]] = distinct !{![[AFTER_VECTOR_10]], ![[ISVECTORIZED:.*]], ![[UNROLL_8:.*]]}
 
 // CHECK: ![[LOOP_11]] = distinct !{![[LOOP_11]], ![[WIDTH_2:.*]], ![[INTERLEAVE_4:.*]], ![[FOLLOWUP_VECTOR_11:.*]]}
 // CHECK: ![[FOLLOWUP_VECTOR_11]] = !{!"llvm.loop.vectorize.followup_all", ![[AFTER_VECTOR_11:.*]]}
 // CHECK: ![[AFTER_VECTOR_11]] = distinct !{![[AFTER_VECTOR_11]], ![[ISVECTORIZED:.*]], ![[UNROLL_8:.*]]}
 
 // CHECK: ![[LOOP_12]] = distinct !{![[LOOP_12]], ![[WIDTH_6:.*]], ![[INTERLEAVE_10:.*]], ![[FOLLOWUP_VECTOR_12:.*]]}
 // CHECK: ![[FOLLOWUP_VECTOR_12]] = !{!"llvm.loop.vectorize.followup_all", ![[AFTER_VECTOR_12:.*]]}
 // CHECK: ![[AFTER_VECTOR_12]] = distinct !{![[AFTER_VECTOR_12]], ![[ISVECTORIZED:.*]], ![[UNROLL_24:.*]]}
 // CHECK: ![[UNROLL_24]] = !{!"llvm.loop.unroll.count", i32 24}
 
-// CHECK: ![[LOOP_13]] = distinct !{![[LOOP_13]], ![[WIDTH_8:.*]], ![[INTERLEAVE_16:.*]], ![[FOLLOWUP_VECTOR_13:.*]]}
+// CHECK: ![[LOOP_13]] = distinct !{![[LOOP_13]], ![[WIDTH_8:.*]], ![[INTERLEAVE_16:.*]], ![[VECTORIZE_ENABLE]], ![[FOLLOWUP_VECTOR_13:.*]]}
 // CHECK: ![[INTERLEAVE_16]] = !{!"llvm.loop.interleave.count", i32 16}
 // CHECK: ![[FOLLOWUP_VECTOR_13]] = !{!"llvm.loop.vectorize.followup_all", ![[AFTER_VECTOR_13:.*]]}
 // CHECK: ![[AFTER_VECTOR_13]] = distinct !{![[AFTER_VECTOR_13]], ![[ISVECTORIZED:.*]], ![[UNROLL_32:.*]]}
 // CHECK: ![[UNROLL_32]] = !{!"llvm.loop.unroll.count", i32 32}
 
-// CHECK: ![[LOOP_14]] = distinct !{![[LOOP_14]], ![[WIDTH_10:.*]]}
+// CHECK: ![[LOOP_14]] = distinct !{![[LOOP_14]], ![[WIDTH_10:.*]], ![[VECTORIZE_ENABLE]]}
 // CHECK: ![[WIDTH_10]] = !{!"llvm.loop.vectorize.width", i32 10}