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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp:53
+Instruction *InstCombiner::visitAtomicRMWInst(AtomicRMWInst &RMWI) {
+  if (!isIdempotentRMW(RMWI))
+    return nullptr;
----------------
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!


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58242/new/

https://reviews.llvm.org/D58242