[PATCH] D118387: [IPSCCP] Switch away from Instruction::isSafeToRemove()

[Kevin P. Neal via Phabricator via llvm-commits]

kpn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:94
 
+static bool canSCCPRemoveInstruction(Instruction *I) {
+  if (wouldInstructionBeTriviallyDead(I))
----------------
sepavloff wrote:
> I would say that `SCCP` in the function name is excessive, because we are already in SCCP.
OK, easily changed.


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:98
+
+  return (!isa<CallInst>(I) || !I->mayHaveSideEffects()) && !I->isTerminator();
+}
----------------
sepavloff wrote:
> kpn wrote:
> > sepavloff wrote:
> > > This check is almost the same as used by `Instruction::isSafeToRemove`, but the latter also checks for `isEHPad`. Why this check is omitted here? Would it be more consistent just to call `isSafeToRemove`?
> > I believe you are correct and this check should not have been omitted.
> > 
> > I'm trying to eliminate isSafeToRemove() since the Instruction class is the wrong place to do this check, and like the summary says, I don't want another implementation of isInstructionTriviallyDead().
> I wonder if you can just make
> ```
>     return I->isSafeToRemove();
> ```
> instead of copying the body of `isSafeToRemove()`?
@sepavloff I think we have a few options:
1) Keep I->isSafeToRemove(), but put wrappers around it. This leaves a land mine for future developers to add more uses of it that are incompatible with the constrained intrinsics.
2) Keep I->isSafeToRemove(), but teach it about the constrained intrinsics. This makes it a copy of wouldInstructionBeTriviallyDead(), except worse. And it creates confusion in the future because a developer may not know which of the two functions is needed.
3) Push the body of I->isSafeToRemove() up into the _two_ callers of the function and then eliminate it. No future confusion, no opportunity to break the constrained intrinsics by using the wrong function.

Since there are only two callers it seems to me it's best to take #3 above.

@fhahn, are you referring to the body of Instruction::isSafeToRemove() that has been lifted up into this function here? We were using I->isSafeToRemove() before so this movement into this new wrapper keeps the behavior pretty much the same. IPSCCP's handling of atomic loads shouldn't be affected since a rejection by wouldInstructionBeTriviallyDead() falls through into the same code as before. Can we build on this to improve wouldInstructionBeTriviallyDead() later?


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

https://reviews.llvm.org/D118387