[PATCH] D41638: [llvm-extract] Support extracting basic blocks

[Justin Bogner via Phabricator via llvm-commits]

bogner added inline comments.


================
Comment at: include/llvm/Transforms/IPO.h:182-188
 /// createBlockExtractorPass - This pass extracts all blocks (except those
 /// specified in the argument list) from the functions in the module.
 ///
 ModulePass *createBlockExtractorPass();
+ModulePass *
+createBlockExtractorPass(const SmallVectorImpl<BasicBlock *> &BlocksToExtract,
+                         bool EraseFunctions);
----------------
The doc comment is either confusing or wrong here, I think. Could you please update it?


================
Comment at: lib/Transforms/IPO/BlockExtractor.cpp:76-77
+  auto ErrOrBuf = MemoryBuffer::getFile(BlockExtractorFile);
+  if (std::error_code EC = ErrOrBuf.getError())
+    llvm_unreachable("BlockExtractor couldn't load the file.");
+  // Read the file.
----------------
Should this use report_fatal_error or something instead of llvm_unreachable?


================
Comment at: lib/Transforms/IPO/BlockExtractor.cpp:134-135
+    Function *F = M.getFunction(BInfo.first);
+    if (!F)
+      llvm_unreachable("Invalid function name specified in the input file");
+    auto Res = std::find_if(F->begin(), F->end(), [&](const BasicBlock &BB) {
----------------
These should either be fatal errors (see above comment) or asserts. "if (x) unreachable" is a strange pattern.


================
Comment at: lib/Transforms/IPO/BlockExtractor.cpp:136-138
+    auto Res = std::find_if(F->begin(), F->end(), [&](const BasicBlock &BB) {
+      return BB.getName().equals(BInfo.second);
+    });
----------------
llvm::find_if would save a few characters, if you prefer.


================
Comment at: lib/Transforms/IPO/BlockExtractor.cpp:155
+      BlocksToExtractVec.push_back(II->getUnwindDest());
+    CodeExtractor(BlocksToExtractVec).extractCodeRegion();
+    ++NumExtracted;
----------------
I guess CodeExtractor takes care of naming the extracted function, but could you double check that it does the right thing for name collisions? IE, if you extract bb1 from foo, but there's already a function called foo_bb1, what happens?


================
Comment at: test/tools/llvm-extract/extract-block.ll:3
+
+; CHECK: @foo_bb4
+define i32 @foo(i32 %arg) {
----------------
Worth checking that it got the right block? Could check for "%tmp5 = ", etc.


================
Comment at: test/tools/llvm-extract/extract-invalid-block.ll:3
+
+; CHECK: function foo doesn't contains basic block named 'invalidbb'!
+define i32 @foo(i32 %arg) {
----------------
typo, s/contains/contain/ (pretty sure this test doesn't pass right now)


================
Comment at: tools/bugpoint/ExtractFunction.cpp:410
 
-  std::string uniqueFN = "--extract-blocks-file=";
+  std::string uniqueFN = "-extract-blocks-file=";
   uniqueFN += Temp->TmpName;
----------------
Why this change?


================
Comment at: tools/llvm-extract/llvm-extract.cpp:241-244
+    if (ExtractFuncs.size() != 1 || !ExtractRegExpFuncs.empty()) {
+      errs() << argv[0]
+             << ": unable to extract basic blocks, multiple functions were "
+                "given to extract!\n";
----------------
Not sure it's worth it, but I wonder if it would make sense to allow something like "-bb func1:bb2 -bb func2:bb3" to support extracting blocks from multiple functions.


https://reviews.llvm.org/D41638