[PATCH] D50670: Implementation of nested loops in cxx_loop_proto

[Matt Morehouse via Phabricator via cfe-commits]

morehouse added a comment.

Does this hit new coverage in the vectorizer?



================
Comment at: clang/tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:46
 std::string VarRefToString(std::ostream &os, const VarRef &x) {
+  std::string var = inner_loop ? "inner" : "outer";
   std::string arr;
----------------
Please choose a better name than var.


================
Comment at: clang/tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:127
   }
+  inner_loop = true;
   return os;
----------------
This looks like a bug.  `inner_loop` never gets set to false again.  Might be a good reason to make it parameter instead.


================
Comment at: clang/tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:131
 std::ostream &operator<<(std::ostream &os, const LoopFunction &x) {
-  return os << "target triple = \"x86_64-unknown-linux-gnu\"\n"
-            << "define void @foo(i32* %a, i32* %b, i32* %c, i64 %s) {\n"
-            << "%1 = icmp sgt i64 %s, 0\n"
-            << "br i1 %1, label %start, label %end\n"
-            << "start:\n"
-            << "br label %loop\n"
-            << "end:\n"
-            << "ret void\n"
-            << "loop:\n"
-            << " %ct   = phi i64 [ %ctnew, %loop ], [ 0, %start ]\n"
-            << x.statements()
-            << "%ctnew = add i64 %ct, 1\n"
-            << "%j = icmp eq i64 %ctnew, %s\n"
-            << "br i1 %j, label %end, label %loop, !llvm.loop !0\n}\n"
-            << "!0 = distinct !{!0, !1, !2}\n"
-            << "!1 = !{!\"llvm.loop.vectorize.enable\", i1 true}\n"
-            << "!2 = !{!\"llvm.loop.vectorize.width\", i32 " << kArraySize
-            << "}\n";
+  os << "target triple = \"x86_64-pc-linux-gnu\"\n"
+     << "define void @foo(i32* %a, i32* %b, i32* noalias %c, i64 %s) {\n"
----------------
Why do you change this to `pc` again?


================
Comment at: clang/tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:132
+  os << "target triple = \"x86_64-pc-linux-gnu\"\n"
+     << "define void @foo(i32* %a, i32* %b, i32* noalias %c, i64 %s) {\n"
+     << "%cmp = icmp sgt i64 %s, 0\n"
----------------
I'm curious how this change affects coverage independent of the rest of this change.  Also what would happen if we set `%a` and `%b` to noalias as well?


================
Comment at: clang/tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:136
+     << "outer_loop_start:\n"
+     << "br label %inner_loop_start\n"
+     << "inner_loop_start:\n"
----------------
Looks like a pointless branch.


================
Comment at: clang/tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:144
+     << x.outer_statements()
+     << "%o_ct_new = add nuw nsw i64 %outer_ct, 1\n"
+     << "%jmp_outer = icmp eq i64 %o_ct_new, %s\n"
----------------
Why `nuw`, `nsw` here?


================
Comment at: clang/tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:154
+     << "}\n"
+     << "!0 = distinct !{!0, !1, !2}\n"
+     << "!1 = !{!\"llvm.loop.vectorize.enable\", i1 true}\n"
----------------
Can we simplify the order of blocks here?  It is confusing to follow all these jumps forward and backward.


Repository:
  rC Clang

https://reviews.llvm.org/D50670