[PATCH] D36274: Introduce FuzzMutate library

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

This looks good to me!



================
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,
----------------
bogner wrote:
> 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?
Sorry, I hadn't fully parsed your second sentence there.


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


================
Comment at: lib/FuzzMutate/IRMutator.cpp:180
+  if (!RS)
+    RS.sample(IB.newSource(*BB, InstsBefore, {}, Pred), /*Weight=*/1);
+
----------------
bogner wrote:
> 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).
I see, it makes sense to bias towards not adding a source, then.


================
Comment at: lib/FuzzMutate/RandomIRBuilder.cpp:63
+    IP = ++I->getIterator();
+  return new LoadInst(Ptr, "L", &*IP);
+}
----------------
bogner wrote:
> 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.
Got it.


https://reviews.llvm.org/D36274