[PATCH] D58242: Teach instcombine about remaining idemptotent atomicrmw types

Philip Reames via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Feb 14 12:58:06 PST 2019

reames marked an inline comment as done.
reames added inline comments.

Comment at: lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp:53
+Instruction *InstCombiner::visitAtomicRMWInst(AtomicRMWInst &RMWI) {
+  if (!isIdempotentRMW(RMWI))
+    return nullptr;
qcolombet wrote:
> reames wrote:
> > qcolombet wrote:
> > > spatel wrote:
> > > > qcolombet wrote:
> > > > > 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).
> > > > 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)
> > > 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.
> > 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?
> 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!
Here are some ideas:


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.  :)




More information about the llvm-commits mailing list