This is an archive of the discontinued LLVM Phabricator instance.

[mips][atomics] Fix partword atomic binary operation implementation
ClosedPublic

Authored by sdardis on Apr 28 2016, 3:00 AM.

Details

Summary

Currently Mips::emitAtomicBinaryPartword() does not properly respect the
width of pointers. For MIPS64 this causes the memory address that the ll/sc
sequence uses to be truncated. At runtime this causes a segmentation fault.

This can be fixed by applying similar changes as r266204, so that a full 64bit
pointer is loaded.

Diff Detail

Event Timeline

sdardis updated this revision to Diff 55391.Apr 28 2016, 3:00 AM
sdardis retitled this revision from to [mips][atomics] Fix partword atomic binary operation implementation.
sdardis updated this object.
sdardis added a reviewer: dsanders.
sdardis set the repository for this revision to rL LLVM.
sdardis added a subscriber: llvm-commits.
sdardis updated this revision to Diff 55396.Apr 28 2016, 3:32 AM
sdardis edited edge metadata.
sdardis removed rL LLVM as the repository for this revision.

Fixed formatting.

dsanders accepted this revision.Apr 28 2016, 8:25 AM
dsanders edited edge metadata.

LGTM with a couple nits.

Also, does this fix any of the machine verifier failures that Quentin reported in https://llvm.org/bugs/show_bug.cgi?id=27458? I think it might fix atomic.ll or atomicops.ll.

From the commit message:
This can be fixed by applying similar changes as D18995, so that a full 64bit pointer is loaded.

Could you refer to the svn revision rather than the phabricator review?

test/CodeGen/Mips/atomicrmw.ll
8–19

This test looks very similar to AtomicLoadAdd16 in atomic.ll can we add checks to that instead? The new 'daddiu ..., $zero, -4' will pass the check in atomic.ll by accident at the moment since CHECK: addiu will match inside daddiu

16

This statement is dead

This revision is now accepted and ready to land.Apr 28 2016, 8:25 AM
sdardis updated this revision to Diff 55426.Apr 28 2016, 9:27 AM
sdardis updated this object.
sdardis edited edge metadata.
sdardis set the repository for this revision to rL LLVM.

Nits addressed.

sdardis closed this revision.Apr 28 2016, 10:54 AM

Committed as r267900.

Unfortunately this patch doesn't fix the issues Quentin pointed out. The instructions here are taking operands of an incorrect register class (GPR32Opnd) rather than GPR64Opnd. I'll submit a patch for that.