Page MenuHomePhabricator

SROA: preserve alignment tags on loads and stores.
Needs ReviewPublic

Authored by t.p.northover on Nov 20 2018, 1:45 AM.



When splitting up an alloca's uses we were dropping any explicit alignment tags on load and store instructions, which means they default to the ABI-required default alignment and this can cause miscompiles if the original value was smaller.

While I was there I decided to move the TBAA data into the base class too, since it was already shared.

Diff Detail


Event Timeline

t.p.northover created this revision.Nov 20 2018, 1:45 AM

Maybe explicitly add reviewers?
git diff --name-only --cached | xargs -n 1 git blame --porcelain | grep "^author " | sort | uniq -c | sort -nr | head -10

Looks ok to me, but i'm not familiar with this code.


Outdated comment?

Maybe explicitly add reviewers?

Good idea, I've found some victims.

chandlerc added inline comments.Dec 4 2018, 6:00 AM

Can you separate this to its own patch please? Happy for it to be first or second.

chandlerc requested changes to this revision.Dec 4 2018, 6:01 AM
This revision now requires changes to proceed.Dec 4 2018, 6:01 AM

Separated NFC alias analysis move out into, which this change applies on top of.

Gerolf added a subscriber: Gerolf.Dec 13 2018, 1:37 PM
Gerolf added inline comments.

Do you need the BaseAlign parameter? It seems at all instances it could be computed inside the OpSplitter from the InsertionPoint and DL using getAdjustedAlignment().

t.p.northover marked an inline comment as done.Dec 14 2018, 3:10 AM
t.p.northover added inline comments.

It's not essential, but I think it makes sense to update the alignment as we go rather than recalculating exactly the same thing repeatedly for each load.

Gerolf accepted this revision.Dec 14 2018, 5:25 PM

This is straightforward way to pass along alignment tags and fixes (at least one critical) bug. LGTM.

t.p.northover accepted this revision.Dec 18 2018, 1:37 AM

Thanks Gerolf. Committed as r349465.

Close this?
(or better yet, do use "Differential Revision: " in the commit message)

Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2019, 10:46 AM