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

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur requested changes to this revision.
Meinersbur added inline comments.
This revision now requires changes to proceed.


================
Comment at: clang/lib/CodeGen/CGLoopInfo.cpp:272-277
+  if (Attrs.VectorizeWidth > 1 &&
+      Attrs.VectorizeEnable == LoopAttributes::Unspecified)
+    Args.push_back(
+        MDNode::get(Ctx, {MDString::get(Ctx, "llvm.loop.vectorize.enable"),
+                          ConstantAsMetadata::get(ConstantInt::get(
+                              llvm::Type::getInt1Ty(Ctx), 1))}));
----------------
[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.


================
Comment at: clang/test/CodeGenCXX/pragma-loop-predicate.cpp:61
 
-
 // CHECK:      ![[LOOP0]] = distinct !{![[LOOP0]], !3}
----------------
unintentional change?


================
Comment at: clang/test/CodeGenCXX/pragma-loop.cpp:161-177
+void vec_width_1(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}vec_width_1{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP_15:.*]]
+
+  #pragma clang loop vectorize(enable) vectorize_width(1)
+  for (int i = 0; i < Length; i++)
+    List[i] = i * 2;
----------------
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?


================
Comment at: clang/test/CodeGenCXX/pragma-loop.cpp:239
+
+// CHECK-NEXT: ![[LOOP_16]] = distinct !{![[LOOP_16]], ![[WIDTH_1]]}
----------------
`-NEXT` should be unnecessary here. I'd even go towards `CHECK-DAG` since the order of the metadata is unimportant.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69628/new/

https://reviews.llvm.org/D69628