[llvm] r216016 - IR: Fix ConstantExpr::replaceUsesOfWithOnConstant()

[Duncan P. N. Exon Smith]

Author: dexonsmith
Date: Tue Aug 19 15:03:35 2014
New Revision: 216016

URL: http://llvm.org/viewvc/llvm-project?rev=216016&view=rev
Log:
IR: Fix ConstantExpr::replaceUsesOfWithOnConstant()

Change `ConstantExpr` to follow the model the other constants are using:
only malloc a replacement if it's going to be used.  This fixes a subtle
bug where if an API user had used `ConstantExpr::get()` already to
create the replacement but hadn't given it any users, we'd delete the
replacement.

This relies on r216015 to thread `OnlyIfReduced` through
`ConstantExpr::getWithOperands()`.

Modified:
    llvm/trunk/include/llvm/IR/Constants.h
    llvm/trunk/lib/IR/Constants.cpp
    llvm/trunk/unittests/IR/ConstantsTest.cpp

Modified: llvm/trunk/include/llvm/IR/Constants.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Constants.h?rev=216016&r1=216015&r2=216016&view=diff
==============================================================================
--- llvm/trunk/include/llvm/IR/Constants.h (original)
+++ llvm/trunk/include/llvm/IR/Constants.h Tue Aug 19 15:03:35 2014
@@ -1142,12 +1142,6 @@ private:
   void setValueSubclassData(unsigned short D) {
     Value::setValueSubclassData(D);
   }
-
-  /// \brief Check whether this can become its replacement.
-  ///
-  /// For use during \a replaceUsesOfWithOnConstant(), check whether we know
-  /// how to turn this into \a Replacement, thereby reducing RAUW traffic.
-  bool canBecomeReplacement(const Constant *Replacement) const;
 };
 
 template <>

Modified: llvm/trunk/lib/IR/Constants.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Constants.cpp?rev=216016&r1=216015&r2=216016&view=diff
==============================================================================
--- llvm/trunk/lib/IR/Constants.cpp (original)
+++ llvm/trunk/lib/IR/Constants.cpp Tue Aug 19 15:03:35 2014
@@ -2857,63 +2857,26 @@ void ConstantExpr::replaceUsesOfWithOnCo
   Constant *To = cast<Constant>(ToV);
 
   SmallVector<Constant*, 8> NewOps;
+  unsigned NumUpdated = 0;
   for (unsigned i = 0, e = getNumOperands(); i != e; ++i) {
     Constant *Op = getOperand(i);
-    NewOps.push_back(Op == From ? To : Op);
+    if (Op == From) {
+      ++NumUpdated;
+      Op = To;
+    }
+    NewOps.push_back(Op);
   }
+  assert(NumUpdated && "I didn't contain From!");
 
-  Constant *Replacement = getWithOperands(NewOps);
-  assert(Replacement != this && "I didn't contain From!");
-
-  // Check if Replacement has no users (and is the same type).  Ideally, this
-  // check would be done *before* creating Replacement, but threading this
-  // through constant-folding isn't trivial.
-  if (canBecomeReplacement(Replacement)) {
-    // Avoid unnecessary RAUW traffic.
-    auto &ExprConstants = getType()->getContext().pImpl->ExprConstants;
-    ExprConstants.remove(this);
-
-    auto *CE = cast<ConstantExpr>(Replacement);
-    for (unsigned I = 0, E = getNumOperands(); I != E; ++I)
-      // Only set the operands that have actually changed.
-      if (getOperand(I) != CE->getOperand(I))
-        setOperand(I, CE->getOperand(I));
-
-    CE->destroyConstant();
-    ExprConstants.insert(this);
+  if (Constant *C = getWithOperands(NewOps, getType(), true)) {
+    replaceUsesOfWithOnConstantImpl(C);
     return;
   }
 
-  // Everyone using this now uses the replacement.
-  replaceAllUsesWith(Replacement);
-
-  // Delete the old constant!
-  destroyConstant();
-}
-
-bool ConstantExpr::canBecomeReplacement(const Constant *Replacement) const {
-  // If Replacement already has users, use it regardless.
-  if (!Replacement->use_empty())
-    return false;
-
-  // Check for anything that could have changed during constant-folding.
-  if (getValueID() != Replacement->getValueID())
-    return false;
-  const auto *CE = cast<ConstantExpr>(Replacement);
-  if (getOpcode() != CE->getOpcode())
-    return false;
-  if (getNumOperands() != CE->getNumOperands())
-    return false;
-  if (getRawSubclassOptionalData() != CE->getRawSubclassOptionalData())
-    return false;
-  if (isCompare())
-    if (getPredicate() != CE->getPredicate())
-      return false;
-  if (hasIndices())
-    if (getIndices() != CE->getIndices())
-      return false;
-
-  return true;
+  // Update to the new value.
+  if (Constant *C = getContext().pImpl->ExprConstants.replaceOperandsInPlace(
+          NewOps, this, From, To, NumUpdated, U - OperandList))
+    replaceUsesOfWithOnConstantImpl(C);
 }
 
 Instruction *ConstantExpr::getAsInstruction() {

Modified: llvm/trunk/unittests/IR/ConstantsTest.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/ConstantsTest.cpp?rev=216016&r1=216015&r2=216016&view=diff
==============================================================================
--- llvm/trunk/unittests/IR/ConstantsTest.cpp (original)
+++ llvm/trunk/unittests/IR/ConstantsTest.cpp Tue Aug 19 15:03:35 2014
@@ -299,5 +299,28 @@ TEST(ConstantsTest, ConstantArrayReplace
   ASSERT_EQ(A01, RefArray->getInitializer());
 }
 
+TEST(ConstantsTest, ConstantExprReplaceWithConstant) {
+  LLVMContext Context;
+  std::unique_ptr<Module> M(new Module("MyModule", Context));
+
+  Type *IntTy = Type::getInt8Ty(Context);
+  Constant *G1 = new GlobalVariable(*M, IntTy, false,
+                                    GlobalValue::ExternalLinkage, nullptr);
+  Constant *G2 = new GlobalVariable(*M, IntTy, false,
+                                    GlobalValue::ExternalLinkage, nullptr);
+  ASSERT_NE(G1, G2);
+
+  Constant *Int1 = ConstantExpr::getPtrToInt(G1, IntTy);
+  Constant *Int2 = ConstantExpr::getPtrToInt(G2, IntTy);
+  ASSERT_NE(Int1, Int2);
+
+  GlobalVariable *Ref =
+      new GlobalVariable(*M, IntTy, false, GlobalValue::ExternalLinkage, Int1);
+  ASSERT_EQ(Int1, Ref->getInitializer());
+
+  G1->replaceAllUsesWith(G2);
+  ASSERT_EQ(Int2, Ref->getInitializer());
+}
+
 }  // end anonymous namespace
 }  // end namespace llvm