This is an archive of the discontinued LLVM Phabricator instance.

MemCpyOpt: combine local load/store sequences into memcpy.
Needs ReviewPublic

Authored by t.p.northover on May 10 2016, 1:18 PM.

Details

Reviewers
junbuml
Summary

We've got a loop idiom recognizer that can spot a memcpy, but as far as I can tell nothing for the case where the user has "helpfully" unrolled their loops for us, leaving a linear load/store sequence.

This pass seemed to have most of the machinery in place already to form memsets, so I adapted it to also create memcpy calls when it could. I've run the test-suite with no issues, but I'm still not really in familiar code so I'd appreciate someone else looking over the code.

Cheers.

Tim.

Diff Detail

Event Timeline

t.p.northover retitled this revision from to MemCpyOpt: combine local load/store sequences into memcpy..
t.p.northover updated this object.
t.p.northover set the repository for this revision to rL LLVM.
t.p.northover added a subscriber: llvm-commits.
majnemer added inline comments.
lib/Transforms/Scalar/MemCpyOptimizer.cpp
579

Atomic?

t.p.northover added inline comments.May 10 2016, 1:24 PM
lib/Transforms/Scalar/MemCpyOptimizer.cpp
579

I don't think MemTransferInsts (i.e. @llvm.mem* calls) can be atomic, can they? For normal loads & stores both volatile and atomic are covered by the isSimple check.

junbuml added inline comments.May 10 2016, 3:41 PM
lib/Transforms/Scalar/MemCpyOptimizer.cpp
158–159

comment should be updated.

206

Looks like it should be :

unsigned MaxIntSize = DL.getLargestLegalIntTypeSize() / 8;

I will submit a separate patch just for this.

615

memset-> memcpy

619–620

Comment should be updated.

t.p.northover removed rL LLVM as the repository for this revision.

Thanks Jun, thought I'd gone through the comments but obviously not all. Updating diff.

t.p.northover marked 3 inline comments as done.May 11 2016, 12:36 PM
junbuml added inline comments.May 11 2016, 12:40 PM
lib/Transforms/Scalar/MemCpyOptimizer.cpp
206

I fix this in D20176.

Just going through my list of open issues and spotted this. Ping?

Tim.

t.p.northover set the repository for this revision to rL LLVM.

Updating patch to ToT.

courbet added a subscriber: courbet.EditedNov 20 2017, 7:04 AM

Hi Tim,

I've started implementing something similar to this revision and stumbled upon this while searching for related things. Analysis suggesta that our codebase would really benefit from this. Are you still willing to submit this (I don't want to duplicate efforts) ?

Note that the patch still applies quite cleanly (only two s/typedef/using/ to reconcile).