This is an archive of the discontinued LLVM Phabricator instance.

[MemCpyOpt] Allow variable lengths in memcpy optimizer
ClosedPublic

Authored by ollef on Apr 20 2021, 10:42 AM.

Details

Summary

This makes the memcpy-memcpy and memcpy-memset optimizations work for
variable sizes as long as they are equal, relaxing the old restriction
that they are constant integers. If they're not equal, the old
requirement that they are constant integers with certain size
restrictions is used.

The implementation works by pushing the length tests further down in the
code, which reveals some places where it's enough that the lengths are
equal (but not necessarily constant).

Diff Detail

Event Timeline

ollef created this revision.Apr 20 2021, 10:42 AM
ollef requested review of this revision.Apr 20 2021, 10:42 AM
Herald added a project: Restricted Project. ยท View Herald TranscriptApr 20 2021, 10:42 AM
Herald added a subscriber: llvm-commits. ยท View Herald Transcript

Looks fine, I'd suggest a few more tests:

  • For the two you already have, add negative tests where the size is dynamic and doesn't match.
  • Add a memcpy from uninit alloca with dynamic size.
llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
1437

I'd move this check a bit higher (accessedBetween is more expensive).

ollef updated this revision to Diff 338962.Apr 20 2021, 12:43 PM

Address review comments.

  • [MemCpyOpt] Remove unused type from tests
  • [MemCpyOpt] Add negative tests for non-equal variable sizes
  • [MemCpyOpt] Move constant int check higher up
  • [MemCpyOpt] Add test for variable sized copy from uninit memory
ollef updated this revision to Diff 338963.Apr 20 2021, 12:45 PM

Fix spelling in file name

Harbormaster completed remote builds in B99781: Diff 338963.
ollef updated this revision to Diff 338966.Apr 20 2021, 12:49 PM
  • [MemCpyOpt] Remove unused type from tests
  • [MemCpyOpt] Add negative tests for non-equal variable sizes
  • [MemCpyOpt] Move constant int check higher up
  • [MemCpyOpt] Add test for variable sized copy from uninit memory
This comment was removed by xbolva00.

I believe I've addressed the reviewer comments and even managed to upload the changes. Still figuring out this Arcanist tool... ๐Ÿ˜…

nikic added inline comments.Apr 20 2021, 1:38 PM
llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
1254โ€“1261

The constant size is only needed for the isMustAlias case below. The other one can work with a dynamic size.

ollef updated this revision to Diff 338988.Apr 20 2021, 1:54 PM
  • [MemCpyOpt] Avoid constant int check when checking for undef content
nikic accepted this revision.Apr 20 2021, 2:26 PM

LGTM

This revision is now accepted and ready to land.Apr 20 2021, 2:26 PM
ollef added a comment.Apr 20 2021, 3:07 PM

Thanks! Can you commit this? I don't think I have commit access.

nikic added a comment.Apr 21 2021, 9:17 AM

@ollef Could you please share the Name <email@address> to use for the commit?

ollef added a comment.Apr 21 2021, 9:53 AM

@nikic: Please use Olle Fredriksson <fredriksson.olle@gmail.com>.

This revision was automatically updated to reflect the committed changes.

Late comment but this broke LUA (could be a mis-compile ) which further broke building sqlite. This commit was reverted in Chrome OS (https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/3066844) but seems like the reporter forgot to mention it upstream.

Chrome OS bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1233594. Seems like it was tcl that was miscompiled, not LUA.