Page MenuHomePhabricator

Canonicalize all "idempotent" atomicrmw ops
ClosedPublic

Authored by reames on Feb 14 2019, 10:59 AM.

Details

Summary

For "idempotent" atomicrmw instructions which we can't simply turn into load, canonicalize the operation and constant. This reduces the matching needed elsewhere in the optimizer, but doesn't directly impact codegen.

For any architecture where OR/Zero is not a good default choice, you can extend the AtomicExpand lowerIdempotentRMWIntoFencedLoad mechanism. I reviewed X86 to make sure this works well, haven't audited other backends.

Diff Detail

Repository
rL LLVM

Event Timeline

reames created this revision.Feb 14 2019, 10:59 AM
jfb requested changes to this revision.Feb 14 2019, 11:03 AM
jfb added inline comments.
lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp
65 ↗(On Diff #186872)

I don't think you want to canonicalize volatile accesses this way.

test/Transforms/InstCombine/atomicrmw.ll
92 ↗(On Diff #186872)

typo

This revision now requires changes to proceed.Feb 14 2019, 11:03 AM
reames updated this revision to Diff 186883.Feb 14 2019, 11:31 AM
reames marked an inline comment as done.

Address review comments

reames added inline comments.Feb 14 2019, 11:35 AM
lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp
65 ↗(On Diff #186872)

Why? It's not changing the memory op implied. I can easy restrict, but why?

jfb accepted this revision.Feb 14 2019, 12:18 PM
jfb added inline comments.
lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp
65 ↗(On Diff #186872)

It's volatile, we just shouldn't touch it. The user asked for something silly, let's do something silly.

This revision is now accepted and ready to land.Feb 14 2019, 12:18 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2019, 12:40 PM