This is an archive of the discontinued LLVM Phabricator instance.

[ExpandMemCmp] Split ExpandMemCmp from CodeGen into its own pass.
ClosedPublic

Authored by courbet on Oct 31 2017, 9:14 AM.

Details

Summary

This is mostly a noop (most of the test diffs are renamed blocks).
There are a few temporary register renames (eax<->ecx) and a few blocks are
shuffled around.

See the discussion in PR33325 for more details.

Diff Detail

Repository
rL LLVM

Event Timeline

courbet created this revision.Oct 31 2017, 9:14 AM
spatel added inline comments.Oct 31 2017, 11:14 AM
lib/CodeGen/TargetPassConfig.cpp
671–675 ↗(On Diff #120993)

Rather than keeping this connected to CGP, how about moving it next to createMergeICmpsPass() above since they are closely related?

courbet updated this revision to Diff 121273.Nov 2 2017, 3:55 AM
  • move ExpandMemCmp closer to MergeICmps
spatel accepted this revision.Nov 2 2017, 7:18 AM

LGTM. See inline for small changes before committing.

lib/CodeGen/TargetPassConfig.cpp
603–607 ↗(On Diff #121273)

It would be good to put a comment here to describe the relationship of these 2 passes. Something like:
The MergeICmpsPass tries to create memcmp calls by grouping sequences of loads and compares. ExpandMemCmpPass then tries to expand those calls into optimally-sized loads and compares. The transforms are enabled by a target lowering hook.

674–676 ↗(On Diff #121273)

Remove the braces (eliminate the diff); coding guidelines say we prefer not to brace one-liners.

test/Transforms/ExpandMemCmp/memcmp.ll
2–3 ↗(On Diff #121273)

I think this is going to run into the same problem I've mentioned before: if the test file is in a generic directory, then bots that do not support the x86 target will fail when trying to run it. You probably need to create an x86 sub-directory with the appropriate lit config file.

This revision is now accepted and ready to land.Nov 2 2017, 7:18 AM
courbet updated this revision to Diff 121302.Nov 2 2017, 7:39 AM
courbet marked 2 inline comments as done.
  • Add comments
  • Make the test run only on X86.
courbet marked an inline comment as done.Nov 2 2017, 8:03 AM

Thanks for the review !

test/Transforms/ExpandMemCmp/memcmp.ll
2–3 ↗(On Diff #121273)

Oops; thanks.

This revision was automatically updated to reflect the committed changes.
courbet marked an inline comment as done.
llvm/trunk/test/CodeGen/X86/memcmp.ll