[llvm-commits] [llvm] r112198 - in /llvm/trunk: lib/Transforms/Scalar/JumpThreading.cpp test/Transforms/JumpThreading/2010-08-26-and.ll

[Owen Anderson]

Fixes are in r112539, except as commented below.  Expanded testcases forthcoming.

On Aug 30, 2010, at 2:15 PM, Chris Lattner wrote:
>> +      SmallVector<std::pair<ConstantInt*, BasicBlock*>, 8> LHSVals;
>> +      ComputeValueKnownInPredecessors(BO->getOperand(0), BB, LHSVals);
>> +      for (unsigned i = 0, e = LHSVals.size(); i != e; ++i) 
>> +        if (LHSVals[i].first == CI)
>> +        Result.push_back(std::make_pair(CI, LHSVals[i].second));
> 
> Indentation.
> 
> I don't follow this logic.  Why do you care if the constant*'s are equal?  All you should care about is that LHSVals[i].first is treated like undef if null, then constant fold the and/or.

The ConstantFold* APIs can't handle this without instantiating a new instruction for each potential incoming value, and then trying to constant fold that.

>> @@ -423,28 +448,63 @@
>>    // If comparing a live-in value against a constant, see if we know the
>>    // live-in value on any predecessors.
>>    if (LVI && isa<Constant>(Cmp->getOperand(1)) &&
>> +        Cmp->getType()->isIntegerTy()) {
>> +      if (!isa<Instruction>(Cmp->getOperand(0)) ||
>> +           cast<Instruction>(Cmp->getOperand(0))->getParent() != BB) {
> 
> Indentation.  Why would you want to accept non-instruction values here?  It seems like the || should be an &&.

Function arguments are the most obvious non-instruction value that the input might have correlated comparisons on.

>> +  if (LVI) {
>> +    // If all else fails, see if LVI can figure out a constant value for us.
>> +    Constant *CI = LVI->getConstant(V, BB);
>> +    ConstantInt *CInt = dyn_cast_or_null<ConstantInt>(CI);
>> +    if (CInt) {
>> +      for (pred_iterator PI = pred_begin(BB), E = pred_end(BB); PI != E; ++PI)
>> +        Result.push_back(std::make_pair(CInt, *PI));
>> +    }
>> +    
>> +    return !Result.empty();
>> +  }
> 
> Is this actually adding value over the code at ~line 300?

The code around line 300 is a special case for handling values not defined in the current block.  The code is question is a last-ditch attempt to figure out (most likely through range propagation) the value of an instruction in the current block.

While they implement the same concept, reusing the line 300 code (looping over the preds, and calling getConstantOnEdge) here would require JumpThreading to know how to join all those values based on the current instruction.  Fortunately, LVI already knows how to do this!

--Owen