This is an archive of the discontinued LLVM Phabricator instance.

Fix an alignment error in `llvm::expandAtomicRMWToCmpXchg`.
AbandonedPublic

Authored by DiamondLovesYou on Aug 5 2015, 2:46 PM.

Download Raw Diff

Details

Reviewers

Summary

Divide the primitive size in bits by eight so the initial load's alignment is in bytes as expected. Tested with the included unit test.

Diff Detail

Repository: rL LLVM

Event Timeline

DiamondLovesYou updated this revision to Diff 31400.Aug 5 2015, 2:46 PM

DiamondLovesYou retitled this revision from to Fix an alignment error in `llvm::expandAtomicRMWToCmpXchg`..

DiamondLovesYou updated this object.

DiamondLovesYou added a reviewer: jfb.

DiamondLovesYou set the repository for this revision to rL LLVM.

DiamondLovesYou added a subscriber: llvm-commits.

lgtm

Adding @morisset and @reames as FYI.

lib/CodeGen/AtomicExpandPass.cpp
553	In a separate patch please also make this atomic! The comment above says that's what the generated code does... but it's not :-)

This revision is now accepted and ready to land.Aug 5 2015, 3:08 PM

jfb edited edge metadata.Aug 5 2015, 3:09 PM

DiamondLovesYou abandoned this revision.Aug 6 2015, 8:53 AM

Revision Contents

Path

Size

lib/

CodeGen/

AtomicExpandPass.cpp

2 lines

test/

Transforms/

AtomicExpand/

expand-atomic-rmw-initial-load.ll

8 lines

Diff 31400

lib/CodeGen/AtomicExpandPass.cpp

Show First 20 Lines • Show All 544 Lines • ▼ Show 20 Lines	bool llvm::expandAtomicRMWToCmpXchg(AtomicRMWInst *AI,

// The split call above "helpfully" added a branch at the end of BB (to the		// The split call above "helpfully" added a branch at the end of BB (to the
// wrong place), but we want a load. It's easiest to just remove		// wrong place), but we want a load. It's easiest to just remove
// the branch entirely.		// the branch entirely.
std::prev(BB->end())->eraseFromParent();		std::prev(BB->end())->eraseFromParent();
Builder.SetInsertPoint(BB);		Builder.SetInsertPoint(BB);
LoadInst *InitLoaded = Builder.CreateLoad(Addr);		LoadInst *InitLoaded = Builder.CreateLoad(Addr);
// Atomics require at least natural alignment.		// Atomics require at least natural alignment.
InitLoaded->setAlignment(AI->getType()->getPrimitiveSizeInBits());		InitLoaded->setAlignment(AI->getType()->getPrimitiveSizeInBits() / 8);
		jfbUnsubmitted Not Done Reply Inline Actions In a separate patch please also make this atomic! The comment above says that's what the generated code does... but it's not :-) jfb: In a separate patch please also make this atomic! The comment above says that's what the…
Builder.CreateBr(LoopBB);		Builder.CreateBr(LoopBB);

// Start the main loop block now that we've taken care of the preliminaries.		// Start the main loop block now that we've taken care of the preliminaries.
Builder.SetInsertPoint(LoopBB);		Builder.SetInsertPoint(LoopBB);
PHINode *Loaded = Builder.CreatePHI(AI->getType(), 2, "loaded");		PHINode *Loaded = Builder.CreatePHI(AI->getType(), 2, "loaded");
Loaded->addIncoming(InitLoaded, BB);		Loaded->addIncoming(InitLoaded, BB);

Value *NewVal =		Value *NewVal =
Show All 20 Lines

test/Transforms/AtomicExpand/expand-atomic-rmw-initial-load.ll

This file was added.

				; RUN: opt -S %s -atomic-expand -mtriple=i686-linux-gnu \| FileCheck %s

				define i8 @test_initial_load(i8* %ptr, i8 %value) {
				%res = atomicrmw nand i8* %ptr, i8 %value seq_cst
				ret i8 %res
				}
				; CHECK-LABEL: @test_initial_load
				; CHECK-NEXT: %1 = load i8, i8* %ptr, align 1