This is an archive of the discontinued LLVM Phabricator instance.

Unpack array of all sizes in InstCombine
ClosedPublic

Authored by deadalnix on Jan 5 2016, 6:22 AM.

Details

Summary

This is another step toward improving fca support. This unpack load of array in a series of load to array's elements.

Diff Detail

Event Timeline

deadalnix updated this revision to Diff 43993.Jan 5 2016, 6:22 AM
deadalnix retitled this revision from to Unpack array of all sizes in InstCombine.
deadalnix updated this object.
deadalnix added a subscriber: llvm-commits.
mgrang added a subscriber: mgrang.Feb 1 2016, 3:48 PM
mgrang added inline comments.
lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
526

Why can't we simply call LI.getName() wherever it is required rather than maintaining the variable "Name"?

531–532

Same here. Do we really need to maintain NumElements? Can we simply call ST->getNumElements() instead?

588

I think these could be written better as:

SmallString<16> LoadName = LI->getName() + ".unpack";
SmallString<16> EltName = LI->getName() + ".elt";

deadalnix added inline comments.Feb 1 2016, 5:37 PM
lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
526

Is that a

526

Is that a problem ? I generally avoid calling the same thing again and again. In this case, it probably doesn't matter much.

588
error: no viable conversion from 'llvm::Twine' to 'SmallString<16>'
deadalnix updated this revision to Diff 48160.Feb 17 2016, 12:59 AM

Fix alignement on generated loads.

deadalnix updated this revision to Diff 48220.Feb 17 2016, 11:25 AM

clang format

majnemer edited edge metadata.Mar 1 2016, 5:32 PM

Does this do the right thing with wacky stuff like arrays of i3?

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
597

Arrays can have more than 2**32 elements.

602

You should use uint64_t here.

deadalnix updated this revision to Diff 49573.Mar 1 2016, 5:51 PM
deadalnix edited edge metadata.

Use 64 bits indices

majnemer accepted this revision.Mar 2 2016, 1:21 PM
majnemer edited edge metadata.

LGTM

This revision is now accepted and ready to land.Mar 2 2016, 1:21 PM
This revision was automatically updated to reflect the committed changes.