[PATCH] D115216: [CodeExtractor] Optionally keep code in original function.

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

Thanks @Meinersbur for this patch. Apologies for the long wait. Just starting to go over it.

The summary is quite helpful to understand the need for this extension to the extractor from the Clang side. I guess this is currently not required for Flang/MLIR? Or is it needed to handle the cancellation constructs?

I think the `KeepFunctions` and `KeepBlocks` flags are useful to ensure that there are no changes to existing users. But I would also like to check with you whether keeping the functions and blocks are necessary for correctness when there are branches to the blocks other than the first one. If so would it be better to support it without flags? Also, do we have asserts presently to ensure that incorrect code is not generated in the presence of these branches when the flags are not specified?



================
Comment at: llvm/include/llvm/Transforms/Utils/Cloning.h:117
 /// parameter.
-BasicBlock *CloneBasicBlock(const BasicBlock *BB, ValueToValueMapTy &VMap,
-                            const Twine &NameSuffix = "", Function *F = nullptr,
-                            ClonedCodeInfo *CodeInfo = nullptr,
-                            DebugInfoFinder *DIFinder = nullptr);
+///
+/// If you would like to clone only a subset of instructions in the basic block,
----------------
Nit: Not about this patch.
DebugInfoFinder is missing from these comments.


================
Comment at: llvm/include/llvm/Transforms/Utils/Cloning.h:120
+/// you can specify a callback returning true only for those instructions that
+/// are to be cloned.
+BasicBlock *
----------------
Nit: There seems to be a style here for mentioning the parameter as the nth (7th here).


================
Comment at: llvm/tools/llvm-extract/llvm-extract.cpp:87
         "If multiple basic blocks are specified in one pair,\n"
-        "the first block in the sequence should dominate the rest.\n"
+        "the first block in the sequence should dominate the rest (Unlsess "
+        "using --bb-keep-blocks).\n"
----------------
Nit: Unlsess -> Unless


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115216