[LLVMdev] LLVM 3.6 and ConstantExpr::replaceUsesOfWithOnConstant(...) strange behavior

[Rafael Espíndola]

On 19 March 2015 at 12:07, Jobin, Gaël <gael.jobin at switzerlandmail.ch> wrote:
> I wrote a pass in LLVM 3.5 and I'm trying to port it into LLVM 3.6.
>
> Unfortunately, my pass replace the operand of some constants expressions
> using the function replaceUsesOfWithOnConstant(...) that have been modified
> in the 3.6 release...
>
>
>
> In both LLVM 3.5 and 3.6, the function
> ConstantExpr::replaceUsesOfWithOnConstant(...) receives 3 arguments : two
> Value and one Use.
>
> In LLVM 3.5, the Use object was not really used. So, I could easily replace
> an operand in a constant expression by giving the old and the new Value as
> arguments with nullptr for Use.
>
> myConstExpr->replaceUsesOfWithOnConstant(oldOp, newOp, nullptr);
>
> Since LLVM 3.6, the Use object is used in a strange manner and the function
> getWithOperands (used inside replaceUsesOfWithOnConstant(..)) take a new
> boolean argument OnlyIfReduced. The documentation says:
>
>   /// If \c OnlyIfReduced is \c true, nullptr will be returned unless
> something
>   /// gets constant-folded, the type changes, or the expression is otherwise
>   /// canonicalized.  This parameter should almost always be \c false.
>
> Strangely, the OnlyIfReduced is set to true as you can see in the source
> code below. Most of time, in my case, the getWithOperands(...) will return
> nullptr because of this new argument (in LLVM 3.5, the old
> getWithOperands(...) did the job).
>
>   if (Constant *C = getWithOperands(NewOps, getType(), true)) {
>     replaceUsesOfWithOnConstantImpl(C);
>     return;
>   }
>
>   // Update to the new value.
>   if (Constant *C =
> getContext().pImpl->ExprConstants.replaceOperandsInPlace(
>           NewOps, this, From, To, NumUpdated, U - OperandList))
>     replaceUsesOfWithOnConstantImpl(C);
>
> Because it return nullptr, the new function replaceOperandsInPlace(...)
> should do the job instead. Unfortunately, there's an "optimization" inside
> replaceOperandsInPlace(...) :
>
>   // Update to the new value.  Optimize for the case when we have a single
>   // operand that we're changing, but handle bulk updates efficiently.
>
>     remove(CP);
>     if (NumUpdated == 1) {
>       assert(OperandNo < CP->getNumOperands() && "Invalid index");
>       assert(CP->getOperand(OperandNo) != To && "I didn't contain From!");
>       CP->setOperand(OperandNo, To);
>     } else {
>       for (unsigned I = 0, E = CP->getNumOperands(); I != E; ++I)
>         if (CP->getOperand(I) == From)
>           CP->setOperand(I, To);
>     }
>
> Normally, the loop inside the else branch should work (iterate over the
> operands and change the old ones by the new ones) but when you have
> NumUpdated equal to 1 (means there's only one operand to change), it
> actually use the Use object to identify the operand number to change
> (????)...
>
>
>
> From my point of view, this behavior is not very logical...
>
>
>
> I see two differents situations:
>
> One want to replace an operand by another one in a constant expression. We
> don't know how many operands are used by the constant expression nor the
> opcode of this constant expression. So, we call
> replaceUsesOfWithOnConstant(...) without Use (should be a default argument
> Use=nullptr) and the function should do the job (iterate over the operands,
> identify those that need to be replaced and replace them). If the
> replacement does not include a constant-folding/type change/canonicalized,
> the function replaceOperandsInPlace(...) is called and the modifications are
> done in place.
> One want to replace only one operand by another one in a constant
> expression. We identified the correct operand so we know in advance the Use
> object linked to the operand and the constant expression. In that case, we
> call replaceUsesOfWithOnConstant with the Use object and the function should
> use the "optimization" inside replaceOperandsInPlace(...) to avoid doing an
> unnecessary loop.
>
> Furthemore, the function replaceOperandsInPlace(...) have defaults arguments
> for NumUpdated=0 and OperandNo=0 that means it can be explicitly called to
> avoid the "optimization" trick.
>
>
>
> Maybe I'm wrong, but for me this implementation seems more correct:
>
>
>
> Source Code
>
>    void ConstantExpr::replaceUsesOfWithOnConstant(Value *From, Value *ToV,
> -                                                 Use *U) {
> +                                                 Use *U = nullptr) {
>      assert(isa<Constant>(ToV) && "Cannot make Constant refer to
> non-constant!");
>      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);
>        if (Op == From) {
>          ++NumUpdated;
>          Op = To;
>        }
>        NewOps.push_back(Op);
>      }
>      assert(NumUpdated && "I didn't contain From!");
>
>
>      if (Constant *C = getWithOperands(NewOps, getType(), true)) {
>        replaceUsesOfWithOnConstantImpl(C);
>        return;
>      }
>
> +    if (U == nullptr) {
> +      NumUpdated = 0;
> +      U = OperandList;
> +    }
>
>      // Update to the new value.
>      if (Constant *C =
> getContext().pImpl->ExprConstants.replaceOperandsInPlace(
>              NewOps, this, From, To, NumUpdated, U - OperandList))
>        replaceUsesOfWithOnConstantImpl(C);
>    }


Sounds reasonable. Can you send a patch and include a test in
unittests/IR/ConstantsTest.cpp?

Thanks,
Rafael