[PATCH] D101577: [InlineCost] Remove visitUnaryInstruction()

[David Li via Phabricator via llvm-commits]

davidxl added a comment.

lgtm



================
Comment at: llvm/lib/Analysis/InlineCost.cpp:1372
-  Value *Operand = I.getOperand(0);
-  if (simplifyInstruction(I, [&](SmallVectorImpl<Constant *> &COps) {
-        return ConstantFoldInstOperands(&I, COps[0], DL);
----------------
aeubanks wrote:
> davidxl wrote:
> > aeubanks wrote:
> > > davidxl wrote:
> > > > Perhaps add  a debug assert that this returns false always (for non-debug path, skip the simplification call for compile time).
> > > there isn't really a good place to put that assert, unless we want to define a `CallAnalyzer::visitUnaryInstruction` only in assert builds, which seems weird
> > The change to in visitAlloca is fine.  To address the concern about future IR (mtrofin), how about keeping visitUnaryInstruction, but with a very simple body -- when it is called in the future, it will be caught and handled here.
> > 
> > {
> >  assert(0);
> >  return false;
> > }
> I'm not sure that it makes sense to special case the assert future instructions that are specifically unary instructions. What about future binary instructions? I'd say it makes more sense to have some assert in visitInstruction(), but I don't think there's some universal assert that can be applied to all potential future instructions.
Fair enough.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101577