This is an archive of the discontinued LLVM Phabricator instance.

[MergeICmps] Simplify the code.
ClosedPublic

Authored by courbet on May 9 2019, 8:37 AM.

Details

Summary

Instead of patching the original blocks, we now generate new blocks and
delete the old blocks. This results in simpler code with a less twisted
control flow (see the change in entry-block-shuffled.ll).

This will make https://reviews.llvm.org/D60318 simpler by making it more
obvious where control flow created and deleted.

Event Timeline

courbet created this revision.May 9 2019, 8:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2019, 8:37 AM
courbet retitled this revision from [MergeICmps][NFC] Re-generate tests with update_test_checks. to [MergeICmps][NFCI] Simplify the code..May 9 2019, 8:39 AM
courbet edited the summary of this revision. (Show Details)
courbet retitled this revision from [MergeICmps][NFCI] Simplify the code. to [MergeICmps] Simplify the code..
courbet updated this revision to Diff 198841.May 9 2019, 8:42 AM

Forgot two test files.

gchatelet added inline comments.May 14 2019, 8:56 AM
llvm/lib/Transforms/Scalar/MergeICmps.cpp
545

Move closer to usage.

550

What's the empty string? Do you mind adding an inline comment?

605

typo on trivial

612

Why not return false; here directly?
Then you don't even need to create a scope.

620

to that the next block?

628

makeArrayRef(Comparisons_).slice(I + 1, NumMerged)
You may want to have a lambda to make this expression easier to understand.

llvm/test/CodeGen/PowerPC/memcmp-mergeexpand.ll
10 ↗(On Diff #198841)

Do you mind explaining what caused the change in the block name?
I can't find a good explanation looking at the code.

courbet marked 8 inline comments as done.May 15 2019, 5:14 AM

Thanks, PTAL.

llvm/lib/Transforms/Scalar/MergeICmps.cpp
550

I'm actually naming the blocks now, so this is no longer relevant.

612

There's a negation :) But good point, I made it more readable.

llvm/test/CodeGen/PowerPC/memcmp-mergeexpand.ll
10 ↗(On Diff #198841)

I'm now naming new blocks as the concatenation of all named blocks. This should make it easier to follow what's happening in tests.

courbet updated this revision to Diff 199584.May 15 2019, 5:14 AM
courbet marked an inline comment as done.
  • address comments
  • update tests with named blocks.
gchatelet accepted this revision.May 15 2019, 5:50 AM
gchatelet added inline comments.
llvm/lib/Transforms/Scalar/MergeICmps.cpp
619

static

629

no braces

This revision is now accepted and ready to land.May 15 2019, 5:50 AM
courbet updated this revision to Diff 199588.May 15 2019, 5:58 AM

Cosmetics.

courbet marked 2 inline comments as done.May 15 2019, 5:58 AM

Thanks.

llvm/lib/Transforms/Scalar/MergeICmps.cpp
619

This is touching Scratch actually :)

This revision was automatically updated to reflect the committed changes.