Page MenuHomePhabricator

[X86] Improve lowering of idemptotent RMW operations
Needs ReviewPublic

Authored by reames on Feb 25 2019, 9:22 AM.

Details

Reviewers
craig.topper
jfb
Summary

The current lowering uses an mfence. mfences are substaintially higher latency than the locked operations originally requested, but we do want to avoid contention on the original cache line. As such, use a locked instruction on a cache line assumed to be thread local.

Diff Detail

Event Timeline

reames created this revision.Feb 25 2019, 9:22 AM
jfb added a comment.Feb 25 2019, 10:56 AM

Overall this looks good, but I have a few comments.

lib/Target/X86/X86ISelLowering.cpp
25482

This comment needs an update.

25492

This seems to need an update too.

test/CodeGen/X86/atomic-idempotent.ll
193

Can you test monotonic as well?
Also, i8 and i16.

jfb added a subscriber: anemet.Feb 25 2019, 10:56 AM
reames planned changes to this revision.Feb 25 2019, 8:31 PM
reames marked 2 inline comments as done.
reames added inline comments.
lib/Target/X86/X86ISelLowering.cpp
25482

I'm not sure what update you're asking for. If we fall down this path, the comments appear correct as they ever were. Something I'm missing?

test/CodeGen/X86/atomic-idempotent.ll
193

Will do.

reames updated this revision to Diff 188385.Feb 26 2019, 8:31 AM

Add tests requested by JF

jfb accepted this revision.Feb 26 2019, 11:44 AM

Overall this LGTM, but that part of CodeGen isn't my area of expertise so it would be nice to get another person to chime in.

This revision is now accepted and ready to land.Feb 26 2019, 11:44 AM
anemet edited reviewers, added: craig.topper; removed: jfb.Feb 26 2019, 1:01 PM
This revision now requires review to proceed.Feb 26 2019, 1:01 PM
anemet added a reviewer: jfb.Feb 26 2019, 1:01 PM
craig.topper added inline comments.Tue, Apr 23, 10:28 PM
lib/Target/X86/X86ISelLowering.cpp
25455

Should that be "auto *C"? I think we try to keep the * on pointers.

25995

Line comments up?

25999

Shouldn't this be mi8 for a shorter encoding?

Does 64-bit really need a 64-bit access or can we just use a 32-bit access?

26081

I think you can use isNullConstant

craig.topper added inline comments.Tue, Apr 23, 10:34 PM
lib/Target/X86/X86ISelLowering.cpp
25984

Windows 64 definitely doesn't have a red zone. I'm not sure if any 32-bit target does.