This is an archive of the discontinued LLVM Phabricator instance.

Fix load alignement when unpacking aggregates structs
ClosedPublic

Authored by deadalnix on Feb 16 2016, 10:24 PM.

Details

Summary

Store and loads unpacked by instcombine do not always have the right alignement. This explicitely compute the alignement and set it.

Diff Detail

Repository
rL LLVM

Event Timeline

deadalnix updated this revision to Diff 48150.Feb 16 2016, 10:24 PM
deadalnix retitled this revision from to Fix load alignement when unpacking aggregates structs.
deadalnix updated this object.
deadalnix added a subscriber: llvm-commits.
majnemer added inline comments.Feb 16 2016, 10:39 PM
lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
977–978 ↗(On Diff #48150)

Would MinAlign from MathExtras work here? I'm thinking MinAlign(SL->getElementOffset(i), Align).

deadalnix updated this revision to Diff 48158.Feb 17 2016, 12:12 AM

Use MinAlign

majnemer accepted this revision.Feb 17 2016, 8:41 AM
majnemer edited edge metadata.

LGTM

This revision is now accepted and ready to land.Feb 17 2016, 8:41 AM
majnemer requested changes to this revision.Feb 17 2016, 8:45 AM
majnemer edited edge metadata.

Actually, some last minute questions came up...

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
548 ↗(On Diff #48158)

I think it'd be more obvious if you used DL.getABITypeAlignment(ST) here seeing as how the langref defines the load's alignment using the language: A value of 0 or an omitted align argument means that the operation has the ABI alignment for the target.

This revision now requires changes to proceed.Feb 17 2016, 8:45 AM
deadalnix updated this revision to Diff 48218.Feb 17 2016, 11:12 AM
deadalnix edited edge metadata.

Use getABITypeAlignment

majnemer accepted this revision.Feb 17 2016, 11:19 AM
majnemer edited edge metadata.

LGTM

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
975–976 ↗(On Diff #48218)

Is this clang-formatted?

This revision is now accepted and ready to land.Feb 17 2016, 11:19 AM
This revision was automatically updated to reflect the committed changes.

Looks like http://reviews.llvm.org/D17158 is backported to 3.8.0 and is causing a segfault. Could this one be backported too?