This is an archive of the discontinued LLVM Phabricator instance.

[CodeExtractor] Refactor extractCodeRegion, fix parameter index confusion.
Needs ReviewPublic

Authored by Meinersbur on Dec 6 2021, 10:09 PM.

Details

Summary

Reorganize the code into phases:

  1. Analyze/normalize
  2. Create extracted function prototype
  3. Generate the new function's implementation
  4. Generate call to new function
  5. Connect call to original function's CFG

The motivation is D115216 to optionally clone the selected code region into the new function instead of moving it. The current structure made it difficult to add such functionality since there was no obvious place to do so, not made easier by some functions doing more than than their name suggests. For instance, constructFunction modifies code outside the constructed function, but also function properties such as setPersonalityFn are derived somewhere else. Another example is emitCallAndSwitchStatement, which despite its name also inserts stores for output parameters.

Many operations also implicitly depend on the order they are applied which this patch tries to reduce. For instance, SwitchCases becomes the list exit blocks which also defines the return value when leaving via that block. It is computed early such that the new function's return instructions and the switch can be generated independently. Also, SwitchCases is combining the lists ExitBlocks and OldTargets which were not always kept consistent with each other or NumExitBlocks. The method recomputeExitBlocks() will update it when necessary.

The coding style partially contradict the current coding standard. For instance some local variable start with lower case letters. I updated some, but not all occurrences to make the diff match at least some lines as unchanged.

The patch D96854 introduced some confusion of function argument indexes this is fixed here as well, hence the patch is not NFC anymore. Tested in modified CodeExtractorTest.cpp. Patch D121061 introduced AllocationBlock, but not all allocas were inserted there.

Diff Detail

Event Timeline

Meinersbur created this revision.Dec 6 2021, 10:09 PM
Meinersbur requested review of this revision.Dec 6 2021, 10:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 6 2021, 10:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2022, 1:22 PM
Meinersbur retitled this revision from [CodeExtractor] Refactor extractCodeRegion. NFCI. to [CodeExtractor] Refactor extractCodeRegion, fix parameter index confusion..Mar 31 2022, 1:28 PM
Meinersbur edited the summary of this revision. (Show Details)
Meinersbur added reviewers: nikic, wsmoses.
Meinersbur edited the summary of this revision. (Show Details)Mar 31 2022, 1:30 PM

ping

llvm/lib/Transforms/Utils/CodeExtractor.cpp
1173–1176

@wsmoses AllocationBlock should have been used here as well.

Sorry, I will not have bandwidth to review this. @RaviNarayanaswamy, @clin1, would you be interested in reviewing this and making sure that it works for you?

wsmoses added inline comments.Nov 10 2022, 1:23 PM
llvm/include/llvm/Transforms/Utils/CodeExtractor.h
248

Update comment since OldTargets appears removed?

llvm/lib/Transforms/Utils/CodeExtractor.cpp
829

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.

1177

@Meinersbur in the fixed version can you add a test that hits this issue?

1468

Can the naming here be consistent between SwitchCases (as the externally visible data structure name) and ExitBlocks from this function.

1577

Can the get switch type functionality here and above be moved to a separate function?

Meinersbur marked 3 inline comments as done.Nov 10 2022, 9:23 PM
Meinersbur added inline comments.
llvm/include/llvm/Transforms/Utils/CodeExtractor.h
248

done

llvm/lib/Transforms/Utils/CodeExtractor.cpp
829

There already is an assertion at line 1567:

assert(NumExitBlocks < 0xffff && "too many exit blocks for switch");
1177

done. ("CodeExtractor.AllocaBlock")

Not that the tests both allocas, independent of clang

1468

done (to recomputeSwitchCases)

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

1577

Refactored out getSwitchType().

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

Meinersbur marked 3 inline comments as done.
  • Rebase
  • Address review
clementval resigned from this revision.Jul 13 2023, 1:44 PM