[PATCH] D36274: Introduce FuzzMutate library

[Vedant Kumar via Phabricator via llvm-commits]

vsk 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,
----------------
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().


================
Comment at: lib/FuzzMutate/IRMutator.cpp:56
+  std::vector<Type *> Types;
+  for (auto Getter : AllowedTypes)
+    Types.push_back(Getter(M.getContext()));
----------------
Use const auto & here? Copying a std::function may not be cheap.


================
Comment at: lib/FuzzMutate/IRMutator.cpp:79
+  IRMutationStrategy::mutate(F, IB);
+  eliminateDeadCode(F);
+}
----------------
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.


================
Comment at: lib/FuzzMutate/IRMutator.cpp:89
+  describeFuzzerAggregateOps(Ops);
+  describeFuzzerVectorOps(Ops);
+  return Ops;
----------------
Is it worth adding test coverage for this?


================
Comment at: lib/FuzzMutate/IRMutator.cpp:119
+
+  // Choose an operation that's constrained be valid for the type of the
+  // source, collect any other sources it needs, and then build it.
----------------
nit, 'to be valid'


================
Comment at: lib/FuzzMutate/IRMutator.cpp:122
+  fuzzerop::OpDescriptor OpDesc = chooseOperation(Srcs[0], IB);
+  for (auto Pred : makeArrayRef(OpDesc.SourcePreds).slice(1))
+    Srcs.push_back(IB.findOrCreateSource(BB, InstsBefore, Srcs, Pred));
----------------
Use const auto & here?


================
Comment at: lib/FuzzMutate/IRMutator.cpp:180
+  if (!RS)
+    RS.sample(IB.newSource(*BB, InstsBefore, {}, Pred), /*Weight=*/1);
+
----------------
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.


================
Comment at: lib/FuzzMutate/RandomIRBuilder.cpp:63
+    IP = ++I->getIterator();
+  return new LoadInst(Ptr, "L", &*IP);
+}
----------------
Out of curiosity, are all sources with pointer type LoadInsts?


https://reviews.llvm.org/D36274