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

[Jobin, Gaël]

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);
> }

 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150319/7ea8a290/attachment.html>