Page MenuHomePhabricator

AllocaInst should store Align instead of MaybeAlign.

Authored by efriedma on May 15 2020, 3:59 PM.



Along the lines of D77454 and D79968. Unlike loads and stores, the default alignment is getPrefTypeAlign, to match the existing handling in various places, including SelectionDAG and InstCombine.

Diff Detail

Event Timeline

efriedma created this revision.May 15 2020, 3:59 PM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
efriedma marked 2 inline comments as done.May 15 2020, 4:11 PM
efriedma added inline comments.

I guess it's worth briefly noting what this is about: if isSized is passed a SmallPtrSet, it uses that set to catch infinitely recursive types (for example, a struct that has itself as a member). Otherwise, it just crashes on such types.

An existing test for alloca caught this, so while I was here I extended the same check to load/store.


I'm not completely confident this is correct; LangRef says the default alignment of a byval argument is target-dependent (not encoded in the datalayout), so it could theoretically by higher than getPrefTypeAlign(). But this matches the current behavior of the code.

jdoerfert accepted this revision.May 15 2020, 9:17 PM

Maybe split the one part off but everything LGTM.


LGTM for committing this separately. I guess it would be perfect if you could add a test as well.

This revision is now accepted and ready to land.May 15 2020, 9:17 PM

I almost forgot, thanks again for fixing all of these!

efriedma marked an inline comment as done.May 16 2020, 2:25 PM
efriedma added inline comments.

Pushed 0ec5f501 with testcases.

This revision was automatically updated to reflect the committed changes.