[PATCH] D50670: Implementation of nested loops in cxx_loop_proto

[Matt Morehouse via Phabricator via llvm-commits]

morehouse added inline comments.


================
Comment at: clang/tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:127
   }
+  inner_loop = true;
   return os;
----------------
emmettneyman wrote:
> 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?
That would be better.


================
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"
----------------
emmettneyman wrote:
> 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`.
Right, sorry not sure what I missed there.


================
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:
> 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.
Can we do that in this patch?  I think it makes sense to be able to generate either single loops or nested loops.


Repository:
  rC Clang

https://reviews.llvm.org/D50670