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

Tue Apr 12 22:15:45 PDT 2022

Meinersbur added a comment.

In D115216#3441487 <https://reviews.llvm.org/D115216#3441487>, @kiranchandramohan wrote:

> 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?

As far as I know it it currently not necessary the OpenMP dialect lowing. Neither does MLIR currently support exceptions and even fir-dev does not support final procedures <https://github.com/flang-compiler/f18-llvm-project/commit/e5c507fc3b07a2a42d2e6ac1dc7988b6e37d50cc> yet. However, support basic block sharing for code size reduction or other purposes might be required in the future.

> 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?

This patch only introduces using `KeepOldBlocks` for `llvm-extract`. I did not want to change the default behaviour for the tool, which is throwing away the parent function. Hence, if there are any branches to moved block are left, those branch instructions are thrown away with its parent function. The "create a call to extracted function" functionality offered by CodeExtractor is unused. Using `--bb-keep-functions` without `--bb-keep-blocks` may result in invalid IR if the `--bb` argument does not specify a proper region. This will be caught by the IR verifier in assert builds at CodeExtractor.cpp:1445. Are you suggesting to add the verifier for non-assert builds as well?

OpenMPIRBuilder will always use `KeepOldBlocks` mode.

================
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,
----------------
kiranchandramohan wrote:
> Nit: Not about this patch.
> DebugInfoFinder is missing from these comments.
`CloneBasicBlock` with `InstSelect` parameter is used in this patch.

The parameter `DIFinder` was added in D33655 and unrelated to this patch.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115216