Page MenuHomePhabricator

[Alignment][NFC] Migrate AtomicExpandPass to Align
ClosedPublic

Authored by gchatelet on Jun 8 2020, 2:35 AM.

Details

Summary

This is a followup on D78403.
I'm unsure about getAtomicOpAlign overloads that take AtomicRMWInst and AtomicCmpXchgInst, shouldn't getAlign provide the correct answer already?

Diff Detail

Event Timeline

gchatelet created this revision.Jun 8 2020, 2:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2020, 2:35 AM

I'm unsure about getAtomicOpAlign overloads that take AtomicRMWInst and AtomicCmpXchgInst, shouldn't getAlign provide the correct answer already?

getAlign() on what? AtomicRMWInst and AtomicCmpXchgInst don't have a getAlign() method. (It might make sense to add.)

llvm/lib/CodeGen/AtomicExpandPass.cpp
1516–1517

Can just use I->getAlign() here.

gchatelet updated this revision to Diff 269472.Jun 9 2020, 3:06 AM
gchatelet marked an inline comment as done.
  • Add getAlign to AtomicCmpXchgInst/AtomicRMWInst and simplify the code

@jyknight does that look like a step in the right direction regarding https://bugs.llvm.org/show_bug.cgi?id=27168?

I'm unsure about getAtomicOpAlign overloads that take AtomicRMWInst and AtomicCmpXchgInst, shouldn't getAlign provide the correct answer already?

getAlign() on what? AtomicRMWInst and AtomicCmpXchgInst don't have a getAlign() method. (It might make sense to add.)

My bad, I should have read PR27168 more carefully before requesting comments. I've added the getAlign() methods and looped @jyknight in.

jfb added a comment.Jun 9 2020, 8:52 AM

Would you update anything in the langref w.r.t. these two instructions? The RMW langref doesn't say anything, and the cmpxchg part says:

The pointer passed into cmpxchg must have alignment greater than or equal to the size in memory of the operand.

You're preserving the natural alignment requirement in the code, but langref seems slightly imprecise.

efriedma accepted this revision.Jun 26 2020, 11:45 AM

LGTM. We probably should update langref, but I don't think we need to block this patch.

llvm/include/llvm/IR/Instructions.h
528

Add a doc comment here?

This revision is now accepted and ready to land.Jun 26 2020, 11:45 AM
gchatelet updated this revision to Diff 274126.Jun 29 2020, 8:01 AM
  • Fix documentation

@jfb @efriedma @jyknight I intend to wait for D81662 to introduce a proper alignment and fix https://bugs.llvm.org/show_bug.cgi?id=27168
Then we can clarify LangRef.

jfb added a subscriber: vish99.Jun 29 2020, 10:21 AM

Adding aliment to these operations will also require updating MergeFuncs /cc @vish99 @hiraditya

In D81369#2120501, @jfb wrote:

Adding aliment to these operations will also require updating MergeFuncs /cc @vish99 @hiraditya

SGTM, I'll make sure I'll loop them in.
I'll go ahead and submit this one in the meantime.
Thank you @jfb

This revision was automatically updated to reflect the committed changes.

@vish99 will your work in https://reviews.llvm.org/D82892
check alignment changes as well?

@vish99 will your work in https://reviews.llvm.org/D82892 check alignment changes as well?

Currently no, since both these instructions don't have an explicit alignment attribute. It can be easily added if the attribute exists.

Thanks for the cleanup!

Belatedly LGTM, and I look forward to your fix to add alignment to atomicrmw/cmpxchg in the future. :)