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