[PATCH] D138339: [FuzzMutate] New strategy `ShuffleBlockStrategy`

[Matt Arsenault via Phabricator via llvm-commits]

arsenm requested changes to this revision.
arsenm added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/FuzzMutate/IRMutator.cpp:315
+  // Shuffle these instructions using topological sort.
+  auto hasAliveParent = [&AliveInsts](Instruction *I) {
+    for (Value *O : I->operands()) {
----------------
Add a comment for the function 


================
Comment at: llvm/lib/FuzzMutate/IRMutator.cpp:318-321
+      if (P && AliveInsts.count(P))
+        return false;
+    }
+    return true;
----------------
The return values look backwards to me?


================
Comment at: llvm/lib/FuzzMutate/IRMutator.cpp:323
+  };
+  auto getAliveChildren = [&AliveInsts](Instruction *I) {
+    SmallSet<Instruction *, 4> Children;
----------------
Comment the function 


================
Comment at: llvm/unittests/FuzzMutate/StrategiesTest.cpp:311
+
+TEST(ShuffleBlockStrategy, Shuffle) {
+  LLVMContext Ctx;
----------------
I think you need an additional test with some loops 


================
Comment at: llvm/unittests/FuzzMutate/StrategiesTest.cpp:366
+
+  auto M = parseAssembly(Source.data(), Ctx);
+  Function *F = &*M->begin();
----------------
Module *M


================
Comment at: llvm/unittests/FuzzMutate/StrategiesTest.cpp:373
+  }
+  srand(Seed);
+  for (int i = 0; i < 100; i++) {
----------------
Use C++11 random functions?


================
Comment at: llvm/unittests/FuzzMutate/StrategiesTest.cpp:378
+      int InstCnt = std::distance(BlockList[i]->begin(), BlockList[i]->end());
+      EXPECT_TRUE(InstCnt == InstCnts[i]);
+    }
----------------
EXPECT_EQ


================
Comment at: llvm/unittests/FuzzMutate/StrategiesTest.cpp:380
+    }
+    EXPECT_TRUE(!verifyModule(*M, &errs()));
+  }
----------------
Probably should be ASSERT_FALSE(verifyModule())


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138339/new/

https://reviews.llvm.org/D138339