Page MenuHomePhabricator

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

Repository
rL LLVM

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 ↗(On Diff #46581)

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

531 ↗(On Diff #46581)

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

582 ↗(On Diff #46581)

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 ↗(On Diff #46581)

Is that a

526 ↗(On Diff #46581)

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

582 ↗(On Diff #46581)
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 ↗(On Diff #49390)

Arrays can have more than 2**32 elements.

602 ↗(On Diff #49390)

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.