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.

Diff Detail

Repository
rL LLVM

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 ↗(On Diff #198841)

Move closer to usage.

550 ↗(On Diff #198841)

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

605 ↗(On Diff #198841)

typo on trivial

612 ↗(On Diff #198841)

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

620 ↗(On Diff #198841)

to that the next block?

628 ↗(On Diff #198841)

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 ↗(On Diff #198841)

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

612 ↗(On Diff #198841)

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
551 ↗(On Diff #199584)

static

561 ↗(On Diff #199584)

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
551 ↗(On Diff #199584)

This is touching Scratch actually :)

This revision was automatically updated to reflect the committed changes.