[PATCH] D45904: [CodeExtractor] Allow extracting blocks with exception handling

[Sergey Dmitriev via Phabricator via llvm-commits]

sdmitriev added inline comments.


================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:207-209
+      if (BB->isEHPad()) {
+        llvm_unreachable("The first block cannot be an unwind block");
+        return {};
----------------
rnk wrote:
> Do not attempt to recover from broken invariants. There are three reasonable options:
> 1. If this condition is possible but undesirable under normal operation, log with LLVM_DEBUG, dbgs(), and return {}.
> 2. Assert that this condition is impossible.
> 3. Use report_fatal_error to assert this condition is possible, but don't compile it out from release builds.
Ok, I will log a message with dbgs(), that will preserve existing behavior. buildExtractionBlockSet was returning an empty list when input list contained a block that was not valid to extract.


================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:217-220
+      if (!Result.count(PBB)) {
+        llvm_unreachable("No blocks in this region may have entries from "
+                         "outside the region except for the first block!");
+        return {};
----------------
rnk wrote:
> Ditto, this should probably be an assert, with the whole loop inside an NDEBUG block like it was before.
This is actually an important part of the validation check which verifies than extraction is legal. So moving it under NDEBUG is undesirable since this code won’t run in release build. I will log a message and return an empty list.


================
Comment at: test/Transforms/CodeExtractor/inline_eh.ll:1
+; RUN: opt < %s -skip-partial-inlining-cost-analysis -partial-inliner -S  | FileCheck %s
+; RUN: opt < %s -skip-partial-inlining-cost-analysis -passes=partial-inliner -S  | FileCheck %s
----------------
rnk wrote:
> Please add a similar test that uses _CxxFrameHandler3 to get coverage of all the code you added to handle cleanupret, catchret, etc.
Sure. I will add such test.


Repository:
  rL LLVM

https://reviews.llvm.org/D45904