[PATCH] D50670: Implementation of nested loops in cxx_loop_proto

[Emmett Neyman via Phabricator via cfe-commits]

emmettneyman added inline comments.


================
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"
----------------
morehouse wrote:
> 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?
`%c` was always supposed to be `noalias`. It got changed accidentally in the last patch. I'm not sure what would change in the coverage if `%a` and `%b` also got set to be `noalias`.

@kcc and I had originally planned for `foo (int *a, int *b, int *__restrict__ c, size_t s)` to be just the first function signature we tried for this fuzz target and to change it up and try different signatures.


================
Comment at: clang/tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:127
   }
+  inner_loop = true;
   return os;
----------------
morehouse wrote:
> Maybe this fixes the bug, but modifying `inner_loop` from different functions is still error-prone.
> 
> Please either make this a scoped variable (with a wrapper class that sets it to true in the constructor and sets it to false in the destructor), or make it a parameter.
Would it be better if `inner_loop` was only modified in the `LoopFunction` operator override? For example:

```
std::ostream &operator<<(std::ostream &os, const LoopFunction &x) {
  inner_loop = false;
  os << "target triple = \"x86_64-unknown-linux-gnu\"\n"
     ....
     << << "outer_loop:\n"
     << x.outer_statements();
  inner_loop = true;
  os << "%o_ct_new = add i64 %outer_ct, 1\n"
     ....
     << "!2 = !{!\"llvm.loop.vectorize.width\", i32 " << kArraySize << "}\n";
  return os;

```
And removed `inner_loop = true;` from the above function?


================
Comment at: clang/tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:140
+     << "br label %inner_loop\n"
+     << "end:\n"
+     << "ret void\n"
----------------
morehouse wrote:
> I don't see any jumps to `end`.  I think this will be an infinite loop.
There are two jumps to `end`, one before entering the `outer_loop` and one at the end of `outer_loop`.


================
Comment at: clang/tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:143
+     << "outer_loop:\n"
+     << x.outer_statements()
+     << "%o_ct_new = add i64 %outer_ct, 1\n"
----------------
morehouse wrote:
> morehouse wrote:
> > IIUC this creates loop structure always like this:
> > 
> > ```
> > for (int i = 0; i < s; i++) {
> >   for (int j = 0; j < s; j++) {
> >     // statements
> >   }
> >   // statements
> > }
> > ```
> > 
> > Maybe not necessary for this patch, but I'm curious if adding statements before the inner loop would exercise different coverage in the vectorizer.
> Will all loops be double-nested now?
Yes, that's correct. It's also worth noting that all the statements in the inner loop will only use `j` to index into the arrays, never `i`. It shouldn't be hard to add outer loop statements before the inner loop. A third `StatementSeq` could be added to the `LoopFunction` proto: one for outer loop statements before the inner loop, one for inner loop statements, and one for outer loop statements after the inner loop.


================
Comment at: clang/tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:143
+     << "outer_loop:\n"
+     << x.outer_statements()
+     << "%o_ct_new = add i64 %outer_ct, 1\n"
----------------
emmettneyman wrote:
> morehouse wrote:
> > morehouse wrote:
> > > IIUC this creates loop structure always like this:
> > > 
> > > ```
> > > for (int i = 0; i < s; i++) {
> > >   for (int j = 0; j < s; j++) {
> > >     // statements
> > >   }
> > >   // statements
> > > }
> > > ```
> > > 
> > > Maybe not necessary for this patch, but I'm curious if adding statements before the inner loop would exercise different coverage in the vectorizer.
> > Will all loops be double-nested now?
> Yes, that's correct. It's also worth noting that all the statements in the inner loop will only use `j` to index into the arrays, never `i`. It shouldn't be hard to add outer loop statements before the inner loop. A third `StatementSeq` could be added to the `LoopFunction` proto: one for outer loop statements before the inner loop, one for inner loop statements, and one for outer loop statements after the inner loop.
Yes, all loops will be double-nested. It shouldn't be difficult to make the inner loop optional though. In `cxx_loop_proto`, the first `Statement_Seq`, `inner_statements`, can be made `optional` and then a different IR structures can be generated depending on whether or not `inner_statements` exists.


Repository:
  rC Clang

https://reviews.llvm.org/D50670