This is an archive of the discontinued LLVM Phabricator instance.

[LoopIdiomRecognize] Add support for memmove. Fixes PR25165
Changes PlannedPublic

Authored by EricWF on Feb 3 2018, 4:02 PM.

Details

Reviewers
chandlerc
bkramer
Summary

This patch adds support for memmove formation to LoopIdiomRecognize.cpp, which fixes llvm.org/PR25165.

The following loops now optimize to a memmove.

void copy(int *begin, int *end, int *out) {
  for (; begin != end; ++begin, ++out)
    *out = *begin;
}

void copy2(int *dest, int *source, int size) {
  for (int i=0; i < size; ++i)
    dest[i] = source[i];
}

void copy3(int *arr) {
  for (int i=0; i < 1023; ++i)
    arr[i] = arr[i+1];
}

Diff Detail

Event Timeline

EricWF created this revision.Feb 3 2018, 4:02 PM
EricWF updated this revision to Diff 132757.Feb 3 2018, 4:37 PM
EricWF edited the summary of this revision. (Show Details)
  • Add more tests.

Some quick comments on the code etc., want to think a bit harder about the loop access checks just to make sure I'm not missing anything. Ben may also have thoughts there.

lib/IR/IRBuilder.cpp
169–186 ↗(On Diff #132757)

Maybe factor this out and share it between MemCpy and MemMove?

lib/Transforms/Scalar/LoopIdiomRecognize.cpp
484

Why the check for HasMemcpy here? Seems a bit odd to check it here rather than later....

1056–1057

Maybe leave a FIXME that we should build an atomic memmove lowering much like we did for memcpy that has specified element-width operations....

EricWF marked 2 inline comments as done.Feb 3 2018, 7:43 PM
EricWF added inline comments.
lib/IR/IRBuilder.cpp
169–186 ↗(On Diff #132757)

Actually, I made a dumb mistake, and missed the already existing CreateMemMove function. I'll be removing this entirely.

Perhaps it still makes sense to factor out the shared code, but that should be a separate commit.

lib/Transforms/Scalar/LoopIdiomRecognize.cpp
484

We checked HasMemcpy || HasMemmove at the top of this block, so it's possible (but unlikely) that we only have memmove. Since we don't have any way to lower the atomic store, it seems logical to report LegalStoreKind::None right away.

EricWF updated this revision to Diff 132760.Feb 3 2018, 7:54 PM
EricWF marked an inline comment as done.
  • Address inline comments.
  • Remove duplicate CreateMemmove function in IrBuilder.h.

@chandlerc All of your review comments have been better addressed in the follow up commit D42890, which adds support for atomic memmove's ,

EricWF updated this revision to Diff 132762.EditedFeb 4 2018, 12:18 AM
  • Avoid unnecessary calculation of non-modifying memory references if we already have already found a modifying memory reference.
EricWF updated this revision to Diff 132763.Feb 4 2018, 12:24 AM

Upload the correct patch for avoiding unnecessary calculation of non-modifying memory references when we already have already found a modifying memory reference.

EricWF updated this revision to Diff 132764.Feb 4 2018, 12:43 AM

Fix silly logic bug in last update. Sorry for the spam.

This transform isn't legal. Consider:

#include <assert.h>
void not_memmove(int *x, int *y, int n) {
  for (int i = 0; i < n; ++i)
    x[i] = y[i];
}
int main() {
  int x[] = { 1, 2, 3, 4, 5, 6 };
  not_memmove(&x[1], &x[0], 5);
  for (int i = 0; i < 6; ++i)
    assert(x[i] == 1);
}
EricWF added a comment.Feb 8 2018, 4:50 PM

This transform isn't legal. Consider:

#include <assert.h>
void not_memmove(int *x, int *y, int n) {
  for (int i = 0; i < n; ++i)
    x[i] = y[i];
}

Woops. Thanks for the correction. I'll attempt to clean this up.

EricWF planned changes to this revision.Feb 8 2018, 4:50 PM