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

[kpn]

Summary

In D115737 I found that I needed to teach Instruction::isSafeToRemove() about strictfp/constrained intrinsics. It was pointed out that this is probably the wrong function to use isInstructionTriviallyDead(). It doesn't make sense to have a "second, worse implementation".

I also believe that the Instruction class is the wrong place for this functionality. The information about whether or not an instruction can be removed is in the transform passes and should stay there.

[nikic]

llvm/lib/Transforms/Scalar/SCCP.cpp

96

I'd drop the TLI pass through here. SCCP should primarily be removing instructions it has simplified itself, and that shouldn't need TLI.

103

I don't understand why we need this remaining code -- why is wouldInstructionBeTriviallyDead not sufficient?

[kpn]

llvm/lib/Transforms/Scalar/SCCP.cpp

96

I didn't realize it wasn't required. Dropping it is certainly much easier.

103

From more study, it looks like you are correct and this bit isn't needed. Which implies that this whole function isn't needed and we can just call wouldInstructionBeTriviallyDead() directly.

[kpn]

No, actually wouldInstructionBeTriviallyDead() rejects some instructions that the previous isSafeToRemove() function allowed. By trusting a true returned by wouldInstructionBeTriviallyDead(), but using the old logic if it returned false, we can make this change NFC. To that end I have also removed the constrained FP logic and I plan on adding it back in probably D115737.

[kpn]

Rebase. Ping.

[sepavloff]

llvm/lib/Transforms/Scalar/SCCP.cpp

98

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?

[kpn]

I realized that going through isInstructionTriviallyDead() means it will handle constrained intrinsics differently than it does now. So this isn't strictly speaking an NFC change.

llvm/lib/Transforms/Scalar/SCCP.cpp

98

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

[kpn]

Update for review comments.

[fhahn]

llvm/lib/Transforms/Scalar/SCCP.cpp

98

Why is this extra code needed?

It looks like wouldInstructionBeTriviallyDead doesn't consider atomic loads from globals as trivially dead, which is causing an issue with IPSCCP as it requires such loads to be removed.

According to https://llvm.org/docs/Atomics.html#atomics-and-ir-optimization, atomic loads from globals can be constant folded, so I *think* wouldInstructionBeTriviallyDead should consider them as trivially dead.

[sepavloff]

llvm/lib/Transforms/Scalar/SCCP.cpp

94

I would say that SCCP in the function name is excessive, because we are already in SCCP.

98

I wonder if you can just make

return I->isSafeToRemove();

instead of copying the body of isSafeToRemove()?

[kpn]

llvm/lib/Transforms/Scalar/SCCP.cpp

94

OK, easily changed.

98

@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?

[fhahn]

llvm/lib/Transforms/Scalar/SCCP.cpp

98

@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?

Yes I meant the inlined code here. Without comment, it is very hard for the reader to understand why it is there and why wouldInstructionBeTriviallyDead is not sufficient. I think it is fine to have some extra checks here before moving them to wouldInstructionBeTriviallyDead, but it should be as minimal as possible, with an explantation and a TODO.

[nikic]

llvm/lib/Transforms/Scalar/SCCP.cpp

98

I've put up https://reviews.llvm.org/D124241 to add the additional check to wouldInstructionBeTriviallyDead(). Unfortunately, this doesn't fully address the problem, because IPSCCP can also remove loads from non-constant globals if it determines that they are actually constant. We would either have to retain a special case here, or we'd have to mark the globals as constant before attempting to remove uses.

[kpn]

Update for review comments.

[kpn]

llvm/lib/Transforms/Scalar/SCCP.cpp

98

Having a special case here seems like a lower risk change. It can always be improved later. It would be nice to get strictfp improvements to IPSCCP unblocked.

[nikic]

llvm/lib/Transforms/Scalar/SCCP.cpp

98

Having the special case here is fine, but please limit it to isa<LoadInst>(I) and add a comment for why it is necessary (atomic loads from globals that are effectively constant, in a way that IPSCCP knows but wouldInstructionBeTriviallyDead does not). The current comment is unclear on why the additional checks are needed (and most of them are not needed).

[kpn]

Update for review comments: eliminate unneeded checks.

[nikic]

This generally looks good to me, but I wonder whether it can be tested? I.e. does this allow eliminating (some) constrained FP intrinsics during SCCP and we can test that? Or does that require additional changes?

llvm/lib/Transforms/Scalar/SCCP.cpp

101

We can't really teach wouldInstructionBeTriviallyDead() about this because it requires SCCP-specific knowledge -- I think a more actionable TODO would be along these lines:

TODO: Mark globals as being constant earlier, sowouldInstructionBeTriviallyDead() knows that atomic loads are safe to remove.

[kpn]

Update comment as requested.

No, the change can't be tested yet. That'll come with D115737 once this gets pushed and I get back to updating that ticket.

[nikic]

LGTM

llvm/lib/Transforms/Scalar/SCCP.cpp

 // Helper to check if \p LV is either overdefined or a constant range with more
 // than a single element. This should cover exactly the same cases as the old
 // ValueLatticeElement::isOverdefined() and is intended to be used in the
 // transition to ValueLatticeElement.
 static bool isOverdefined(const ValueLatticeElement &LV) {
   return !LV.isUnknownOrUndef() && !isConstant(LV);
 }
 
+static bool canRemoveInstruction(Instruction *I) {
+  if (wouldInstructionBeTriviallyDead(I))
+    return true;
+
+  // Some instructions can be handled but are rejected above. Catch
+  // those cases by falling through to here.
+  // TODO: Mark globals as being constant earlier, so
+  // TODO: wouldInstructionBeTriviallyDead() knows that atomic loads
+  // TODO: are safe to remove.
+  return isa<LoadInst>(I);
+}
+
 static bool tryToReplaceWithConstant(SCCPSolver &Solver, Value *V) {
   Constant *Const = nullptr;
   if (V->getType()->isStructTy()) {
     std::vector<ValueLatticeElement> IVs = Solver.getStructLatticeValueFor(V);
     if (llvm::any_of(IVs, isOverdefined))
       return false;
     std::vector<Constant *> ConstVals;
     auto *ST = cast<StructType>(V->getType());
static bool tryToReplaceWithConstant(SCCPSolver &Solver, Value *V) {
   }
   assert(Const && "Constant is nullptr here!");
 
   // Replacing `musttail` instructions with constant breaks `musttail` invariant
   // unless the call itself can be removed.
   // Calls with "clang.arc.attachedcall" implicitly use the return value and
   // those uses cannot be updated with a constant.
   CallBase *CB = dyn_cast<CallBase>(V);
-  if (CB && ((CB->isMustTailCall() && !CB->isSafeToRemove()) ||
+  if (CB && ((CB->isMustTailCall() &&
+              !canRemoveInstruction(CB)) ||
              CB->getOperandBundle(LLVMContext::OB_clang_arc_attachedcall))) {
     Function *F = CB->getCalledFunction();
 
     // Don't zap returns of the callee
     if (F)
       Solver.addToMustPreserveReturnsInFunctions(F);
 
     LLVM_DEBUG(dbgs() << "  Can\'t treat the result of call " << *CB
static bool simplifyInstsInBlock(SCCPSolver &Solver, BasicBlock &BB,
                                  SmallPtrSetImpl<Value *> &InsertedValues,
                                  Statistic &InstRemovedStat,
                                  Statistic &InstReplacedStat) {
   bool MadeChanges = false;
   for (Instruction &Inst : make_early_inc_range(BB)) {
     if (Inst.getType()->isVoidTy())
       continue;
     if (tryToReplaceWithConstant(Solver, &Inst)) {
-      if (Inst.isSafeToRemove())
+      if (canRemoveInstruction(&Inst))
         Inst.eraseFromParent();
 
       MadeChanges = true;
       ++InstRemovedStat;
     } else if (isa<SExtInst>(&Inst)) {
       Value *ExtOp = Inst.getOperand(0);
       if (isa<Constant>(ExtOp) || InsertedValues.count(ExtOp))
         continue;