Page MenuHomePhabricator

Teach instcombine about remaining idemptotent atomicrmw types
ClosedPublic

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

Details

Summary

Expand on Quentin's r353471 patch which converts some atomicrmws into loads. Handle remaining operation types, and fix a slight bug. Atomic loads are required to have alignment. Since this was within the InstCombine fixed point, somewhere else in InstCombine was adding alignment before the verifier saw it, but still, we should fix.

Terminology wise, I'm using the "idempotent" naming that is used for the same operations in AtomicExpand and X86ISelLoweringInfo. Once this lands, I'll add similar tests for AtomicExpand, and move the pattern match function to a common location. Any suggestions as to where?

The last thing we should probably do - not in this patch - is to canonicalize the operation for any idempotent atomicrmw which we can't convert to a load. I was thinking to have "atomicrmw or, ptr, 0" be the canonical choice. Thoughts?

Diff Detail

Repository
rL LLVM

Event Timeline

reames created this revision.Feb 14 2019, 9:25 AM

LGTM, one nitpick below.

Nice catch for the alignment stuff!

lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp
53 ↗(On Diff #186858)

IIRC to be pedantic idempotent is when something doesn't change when multiplied by itself, hence this name could be misleading (though I could remember wrong :P).

qcolombet accepted this revision.Feb 14 2019, 9:52 AM
This revision is now accepted and ready to land.Feb 14 2019, 9:52 AM
spatel added inline comments.Feb 14 2019, 10:01 AM
lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp
53 ↗(On Diff #186858)

Idempotent is (copied from Instruction.h):

///   Idempotent operators satisfy:  x op x === x
///
/// In LLVM, the And and Or operators are idempotent.

We have similar logic/switch for binop instructions. See:
ConstantExpr::getBinOpIdentity()
(not sure if there's a way to share code between this and that)

qcolombet added inline comments.Feb 14 2019, 10:25 AM
lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp
53 ↗(On Diff #186858)

Thanks for the reference Sanjay.
Technically, this does not match what we are doing here: x op cst === x where cst may be different than x, but I guess this is a wildly spread language abuse that people just get it.

jfb accepted this revision.Feb 14 2019, 10:36 AM

lgtm

For the idempotent RMW, atomicrmw or, ptr, 0 sgtm. I assume that you'd do the same for floating-point (with a cast)? I wonder if in the long run we'd actually just want a separate instruction for this...

This revision was automatically updated to reflect the committed changes.
reames marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2019, 10:40 AM
reames added inline comments.Feb 14 2019, 10:40 AM
lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp
53 ↗(On Diff #186858)

Seems like there's consensus the name is confusing. Given we use the same naming in AtomicExpand, I'm going to land as is, and then change everything in one go. Any suggestions on better names?

qcolombet added inline comments.Feb 14 2019, 10:45 AM
lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp
53 ↗(On Diff #186858)

To be honest, we can leave it this way. I could be wrong be I guess most people are not confused by this.

If I were to come up with a different name, I would try to convey the notion that this does not change the input value. IsNoOp could be a candidate, but it kind of implies that we can just remove the instruction which is not true...

Long story short, I don't have a good suggestion!

reames marked an inline comment as done.Feb 14 2019, 12:58 PM
reames added inline comments.
lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp
53 ↗(On Diff #186858)

Here are some ideas:
AtomicRMWInst::isIdempotent()
AtomicRMWInst::isMemoryIdempotent()
Instruction::isMemoryIdempotent()
Instruction::isMemoryNop()
Instruction::mayChangeMemory()

ValueTracking.h
mayChangeMemory(Instruction&)

I'm leaning towards mayChangeMemory on Instruction myself.

p.s. I need a common location, regardless of what name we choose. So I'd like the name to be non-confusing. :)