Page MenuHomePhabricator

[NFC] Adding the align attribute on Atomic{CmpXchg|RMW}Inst
ClosedPublic

Authored by gchatelet on Jul 3 2020, 9:43 AM.

Details

Summary

This is the first step to add support for the align attribute to AtomicRMWInst and AtomicCmpXchgInst.
Next step is to add support in IRBuilder and BitcodeReader.
Bug: https://bugs.llvm.org/show_bug.cgi?id=27168

Diff Detail

Event Timeline

gchatelet created this revision.Jul 3 2020, 9:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2020, 9:43 AM
jfb added inline comments.Jul 3 2020, 11:34 AM
llvm/include/llvm/IR/IRBuilder.h
1742

Are types always rounded to a power of two at this point?

i.e. what does this do: struct { char c[3]; }; ?

Also, I think this is wrong for non-lock-free types. The alignment requirement is lower on those.

jyknight accepted this revision.Mon, Jul 6, 12:55 PM

It's odd to have CreateAtomicCmpXchg and CreateAtomicRMW not have Align as an argument when the constructors do, but as long as the migration is finished in an upcoming commit, this seems fine as a step on the path.

llvm/include/llvm/IR/IRBuilder.h
1742

This is just encoding the pre-existing behavior -- you cannot currently create an cmpxchg instruction with alignment other than the size of the type.

Right now, you _also_ cannot create a cmpxchg instruction with other than integral or pointer types, which -- in any _current_ llvm backend, afaik -- have non-power-of-2 sizes.

Upcoming changes will plumb through a required alignment argument everywhere, and then we'll be rid of this weird hardcoded special alignment behavior here.

This revision is now accepted and ready to land.Mon, Jul 6, 12:55 PM
jfb added inline comments.Mon, Jul 6, 2:48 PM
llvm/include/llvm/IR/IRBuilder.h
1742

That sounds good to me. FWIW I checked and we get the following today:

%3 = bitcast %"struct.std::__1::atomic"* %0 to i32*
%4 = zext i24 %1 to i32
%5 = cmpxchg i32* %3, i32 %4, i32 %4 seq_cst seq_cst
ret void

That being said, if we're going to allow other things to come into a cmpxchg in the future (i.e. remove the need to bitcast) then I want to make sure we encode the right requirements here, right now. I agree that they're enforced later in the code when the instruction is created, but that won't always be the case.

gchatelet marked an inline comment as done.Tue, Jul 7, 2:50 AM
gchatelet added inline comments.
llvm/include/llvm/IR/IRBuilder.h
1742

With this patch, the alignment is still not user accessible, just defined a bit higher in the call hierarchy so I think it's fine. This patch is NFC.
Next patch will make this change visible to users so we'll have to document it in LangRef, @jfb I'll loop you in so you can proofread it.

This revision was automatically updated to reflect the committed changes.