Page MenuHomePhabricator

SROA: Don't drop atomic load/store alignments (PR45010)
ClosedPublic

Authored by hans on Feb 27 2020, 8:47 AM.

Details

Summary

SROA will drop the explicit alignment on allocas when the ABI guarantees enough alignment. Because the alignment on new load/store instructions are set base on the alloca's alignment, that means SROA would end up dropping the alignment from atomic loads and stores, which is not allowed (see bug). For those, make sure to always carry over the alignment from the previous instruction.

Diff Detail

Event Timeline

hans created this revision.Feb 27 2020, 8:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 27 2020, 8:47 AM
Herald added subscribers: jfb, hiraditya. · View Herald Transcript
aganea added a subscriber: aganea.Feb 27 2020, 8:49 AM
gchatelet requested changes to this revision.Feb 27 2020, 8:56 AM

Thank you for taking a look @hans !
Only two nits on my side.

llvm/lib/Transforms/Scalar/SROA.cpp
2523

NewLI->setAlignment(LI.getAlign());

2715

NewSI->setAlignment(SI.getAlign());

This revision now requires changes to proceed.Feb 27 2020, 8:56 AM
hans updated this revision to Diff 246975.Feb 27 2020, 9:07 AM
hans marked 2 inline comments as done.

Using getAlign().

I would prefer to just avoid creating unaligned allocas in the first place. https://github.com/llvm/llvm-project/blob/740ed617f7d4d16e7883636c5eff994f8be7eba4/llvm/lib/Transforms/Scalar/SROA.cpp#L4163 has some code which intentionally uses an unspecified alignment... but really, there's no reason to do that. allocas with no specified alignment are implicitly aligned using the ABI type alignment. So it's semantically equivalent to std::max(Alignment, DL.getABITypeAlignment(SliceTy), just more annoying to deal with.

Really, the alignment of load/store/alloca shouldn't be a MaybeAlign in the first place, but someone needs to go through and clean up a bunch of places to make that happen.

bkramer accepted this revision.Feb 27 2020, 9:12 AM

makes sense to me

hans added a comment.Feb 27 2020, 9:19 AM

I would prefer to just avoid creating unaligned allocas in the first place. https://github.com/llvm/llvm-project/blob/740ed617f7d4d16e7883636c5eff994f8be7eba4/llvm/lib/Transforms/Scalar/SROA.cpp#L4163 has some code which intentionally uses an unspecified alignment... but really, there's no reason to do that. allocas with no specified alignment are implicitly aligned using the ABI type alignment. So it's semantically equivalent to std::max(Alignment, DL.getABITypeAlignment(SliceTy), just more annoying to deal with.

The current behaviour is from https://github.com/llvm/llvm-project/commit/903790eff54f473fca8fbf6915713751a070b5df

Changing that seems like a riskier change (I want this for 10.x) whereas copying over the alignment from the store/load seems straight-forward to me.

jfb added inline comments.Feb 27 2020, 9:28 AM
llvm/test/Transforms/SROA/alignment.ll
242

From the code, it looks like you want a test where the load is volatile only as well?
Should probably test stores too?

Does this affect other memory operations, i.e. the atomic ones?

efriedma accepted this revision.Feb 27 2020, 9:37 AM

Changing that seems like a riskier change (I want this for 10.x) whereas copying over the alignment from the store/load seems straight-forward to me.

It's not completely obvious at first glance that the alloca is actually sufficiently aligned. But looking a little more carefully, it should be; we're preserving the alignment of the original alloca. So this is fine, I guess. LGTM

gchatelet accepted this revision.Feb 27 2020, 12:06 PM

Please address @jfb's comment but otherwise LGTM.

This revision is now accepted and ready to land.Feb 27 2020, 12:06 PM
hans marked an inline comment as done.Feb 28 2020, 1:38 AM
hans added inline comments.
llvm/test/Transforms/SROA/alignment.ll
242

I don't think the volatile-only case is interesting. My code doesn't change anything for that case.

I've made the test cover stores too.

I'm not sure what you mean by "other memory operations, i.e. the atomic ones". This change affects exactly the atomic memory operations..

This revision was automatically updated to reflect the committed changes.
jfb added a comment.Feb 28 2020, 8:42 AM

I meant compare and exchange, as well as RMW.

hans added a comment.Mar 2 2020, 2:32 AM
In D75266#1898238, @jfb wrote:

I meant compare and exchange, as well as RMW.

It doesn't look like SROA handles cmpxchg or atomicrmw instructions.

jfb added a comment.Mar 2 2020, 10:37 AM
In D75266#1898238, @jfb wrote:

I meant compare and exchange, as well as RMW.

It doesn't look like SROA handles cmpxchg or atomicrmw instructions.

I know, but if it grows that capability then the author will thank you for adding a test that prevents them from regressing correctness.