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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
