This is an archive of the discontinued LLVM Phabricator instance.

Improve load to store => memcpy
ClosedPublic

Authored by deadalnix on Jan 24 2016, 11:41 AM.

Details

Summary

This now try to reorder instructions in order to help create the optimizable pattern.

Diff Detail

Event Timeline

deadalnix updated this revision to Diff 45832.Jan 24 2016, 11:41 AM
deadalnix retitled this revision from to Imporove load to store => memcpy.
deadalnix updated this object.
mehdi_amini edited edge metadata.Jan 25 2016, 10:44 AM

I haven't looked into details, but I am a bit concerned by the number of nested level and the added length to this function, do you think it would make sense to refactor with one or multiple helper functions?

Indeed, I considered that part of this could be extract in an utility class. I wasn't really sure where to put it, but if you have a suggestion I'm taking. The TL;DR of the code is that it tries to lift an instruction + whatever depends on this instruction above another instruction in the same basic block. It needs to have alias analysis available to do it.

deadalnix updated this revision to Diff 47538.Feb 10 2016, 2:58 PM
deadalnix edited edge metadata.

Ping

What about starting by extracting it as a static helper function and find it a better home later if any other client needs it?

lib/Transforms/Scalar/MemCpyOptimizer.cpp
575

std::any_of?

584

std::any_of?

deadalnix updated this revision to Diff 47889.Feb 12 2016, 7:30 PM

Extract the logic to lift up the store in its own method

deadalnix retitled this revision from Imporove load to store => memcpy to Improve load to store => memcpy.Feb 14 2016, 6:04 PM
deadalnix updated this revision to Diff 50659.Mar 14 2016, 3:18 PM

Use range for loops

mehdi_amini accepted this revision.Mar 14 2016, 3:33 PM
mehdi_amini edited edge metadata.

Looks good to me with a few comments below.

lib/Transforms/Scalar/MemCpyOptimizer.cpp
508

Update the comment: according to the code, it does more than keeping only the *store* args. It contains all the instructions that needs to be placed before P to satisfy the lift.

525

Should be replaceable with something like: for(auto *C : reverse(make_range(P->getIterator(), --SI->getIterator())))

540

Minor suggestion: I think this can be written NeedList ||= std::any_of(...

This revision is now accepted and ready to land.Mar 14 2016, 3:33 PM
deadalnix updated this revision to Diff 50661.Mar 14 2016, 3:34 PM
deadalnix edited edge metadata.

Undo the range loop in moveUp as I reverse the loop direction invonlutary

deadalnix updated this revision to Diff 50669.Mar 14 2016, 3:52 PM

Update comment to reflect what the code do accurately

This revision was automatically updated to reflect the committed changes.