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

[Duncan Exon Smith]

> On Mar 20, 2015, at 6:34 AM, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
> 
>> 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

AFK right now, but IIRC, this function is supposed to be internal to replaceAllUsesWith() and it's not really supposed to be called directly.  Constants aren't supposed to change. 

Why can't you just call replaceAllUsesWith()?

-- dpnes