[PATCH] D36274: Introduce FuzzMutate library

[Justin Bogner via Phabricator via llvm-commits]

bogner added inline comments.


================
Comment at: include/llvm/FuzzMutate/RandomIRBuilder.h:53
+  /// Create a user for \c V in \c BB.
+  void newSink(BasicBlock &BB, ArrayRef<Instruction *> Insts, Value *V);
+  Value *findPointer(BasicBlock &BB, ArrayRef<Instruction *> Insts,
----------------
vsk wrote:
> Since connectToSink() can create a store, its comment should say it does more than just finding a sink. There may also be an opportunity here to fold newSink() into connectToSink().
Its comment already says it may create a new instruction and use that... Maybe I'm misunderstanding you here?


================
Comment at: lib/FuzzMutate/IRMutator.cpp:79
+  IRMutationStrategy::mutate(F, IB);
+  eliminateDeadCode(F);
+}
----------------
vsk wrote:
> Wdyt of looping here until we're sure that the injected IR isn't DCE'd? If it's expensive to evaluate a mutation, we might be able to run faster if we're sure each mutation has done work.
The injected IR is never DCE'd - the point of the "sink" concept is that its something that isn't dead.


================
Comment at: lib/FuzzMutate/IRMutator.cpp:89
+  describeFuzzerAggregateOps(Ops);
+  describeFuzzerVectorOps(Ops);
+  return Ops;
----------------
vsk wrote:
> Is it worth adding test coverage for this?
I don't think there's much point - the check would just be essentially the same list of ops.


================
Comment at: lib/FuzzMutate/IRMutator.cpp:180
+  if (!RS)
+    RS.sample(IB.newSource(*BB, InstsBefore, {}, Pred), /*Weight=*/1);
+
----------------
vsk wrote:
> Wdyt of always considering adding a new source? It'd be sampled along with the other options, with the same weight. I expect that this would increase coverage without causing chaos.
Since the point of the InstDeleter is to decrease size I think it's better as is. When we have to add a new source the size of the IR stays about the same (remove one instruction, add another).


================
Comment at: lib/FuzzMutate/Operations.cpp:43
+  Ops.push_back(cmpOpDescriptor(1, Instruction::ICmp, CmpInst::ICMP_SLT));
+  Ops.push_back(cmpOpDescriptor(1, Instruction::ICmp, CmpInst::ICMP_SLE));
+}
----------------
vsk wrote:
> Have you considered using the FIRST_ICMP_PREDICATE and LAST_ICMP_PREDICATE macros from InstrTypes.h to condense this?
In my previous update I tried this, but I've ended up undoing it now - casting back and forth between the enum values and int to do a loop ends up being uglier than just listing the things.


================
Comment at: lib/FuzzMutate/RandomIRBuilder.cpp:63
+    IP = ++I->getIterator();
+  return new LoadInst(Ptr, "L", &*IP);
+}
----------------
vsk wrote:
> Out of curiosity, are all sources with pointer type LoadInsts?
Not quite, LoadInsts don't have pointer type (they load from a pointer, so you get the pointee type). Currently we only create sources that are some constant or a load from some pointer though, which is what I think you're getting at.


https://reviews.llvm.org/D36274