[PATCH] D115218: [CodeExtractor] Refactor extractCodeRegion, fix parameter index confusion.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur marked 3 inline comments as done.
Meinersbur added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Utils/CodeExtractor.h:248
 
+    /// Updates the list of exit blocks (OldTargets and ExitBlocks) after
+    /// changes of the control flow or the Blocks list.
----------------
wsmoses wrote:
> Update comment since OldTargets appears removed?
done


================
Comment at: llvm/lib/Transforms/Utils/CodeExtractor.cpp:829
+  default:
+    RetTy = Type::getInt16Ty(Context);
+    break;
----------------
wsmoses wrote:
> Is it worth here asserting that the numexitblocks fits in an Int16, I've seen some real c++ codes that actually emit an absurd number of blocks.
There already is an assertion at line 1567:
```
assert(NumExitBlocks < 0xffff && "too many exit blocks for switch");
```


================
Comment at: llvm/lib/Transforms/Utils/CodeExtractor.cpp:1177
-                       &codeReplacer->getParent()->front().front());
-      ReloadOutputs.push_back(alloca);
-      params.push_back(alloca);
----------------
wsmoses wrote:
> @Meinersbur in the fixed version can you add a test that hits this issue?
done. ("CodeExtractor.AllocaBlock")

Not that the tests both allocas, independent of clang


================
Comment at: llvm/lib/Transforms/Utils/CodeExtractor.cpp:1468
 
-  // The new function needs a root node because other nodes can branch to the
-  // head of the region, but the entry node of a function cannot have preds.
-  BasicBlock *newFuncRoot = BasicBlock::Create(header->getContext(),
-                                               "newFuncRoot");
-  auto *BranchI = BranchInst::Create(header);
-  // If the original function has debug info, we have to add a debug location
-  // to the new branch instruction from the artificial entry block.
-  // We use the debug location of the first instruction in the extracted
-  // blocks, as there is no other equivalent line in the source code.
-  if (oldFunction->getSubprogram()) {
-    any_of(Blocks, [&BranchI](const BasicBlock *BB) {
-      return any_of(*BB, [&BranchI](const Instruction &I) {
-        if (!I.getDebugLoc())
-          return false;
-        BranchI->setDebugLoc(I.getDebugLoc());
-        return true;
-      });
-    });
+void CodeExtractor::recomputeExitBlocks() {
+  SwitchCases.clear();
----------------
wsmoses wrote:
> Can the naming here be consistent between `SwitchCases` (as the externally visible data structure name) and `ExitBlocks` from this function.
done (to `recomputeSwitchCases`)

Not that I also removed `NumExitBlocks` which was redundant with `SwitchCases.size()`. Increases the patch delta though.


================
Comment at: llvm/lib/Transforms/Utils/CodeExtractor.cpp:1577
+      brVal = ConstantInt::get(Type::getInt16Ty(Context), SuccNum);
+      break;
+    }
----------------
wsmoses wrote:
> Can the get switch type functionality here and above be moved to a separate function?
Refactored out `getSwitchType()`.

However, this does not get rid of this switch as you might have hoped.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115218