[llvm-commits] [llvm] r52520 - /llvm/trunk/lib/Transforms/Scalar/SCCP.cpp

[Dan Gohman]

On Fri, June 20, 2008 12:57 am, Matthijs Kooijman wrote:
> Yet, isn't the handling it does now (mainly tracking return values) a
> duplicate effort with IPConstProp? Or does SCCP do other things that make
> it
> more useful than IPCP for this stuff?

Yes, and yes :-).

>
>> +void SCCPSolver::visitExtractValueInst(ExtractValueInst &EVI) {
>> +  Value *Aggr = EVI.getOperand(0);
> You'd better use getAggregateOperand() for that?

fixed.

>
>
>> +
>> +  // If the operand to the getresult is an undef, the result is undef.
>> +  if (isa<UndefValue>(Aggr))
>> +    return;
>> +
>> +  // Currently only handle single-index extractvalues.
>> +  if (EVI.getNumIndices() != 1) {
>> +    markOverdefined(&EVI);
>> +    return;
>> +  }
>> +
>> +  Function *F = 0;
>> +  if (CallInst *CI = dyn_cast<CallInst>(Aggr))
>> +    F = CI->getCalledFunction();
>> +  else if (InvokeInst *II = dyn_cast<InvokeInst>(Aggr))
>> +    F = II->getCalledFunction();
>> +
>> +  // TODO: If IPSCCP resolves the callee of this function, we could
>> propagate a
>> +  // result back!
> What do you mean by this? Are you considering the case where you visit the
> extractvalue instruction on the result before the insertvalue instruction
> for
> the return value?

Theoretically IPSCCP could determine the callee of an indirect call
in some cases, and either convert it into a direct call or handle it
as a direct call for the purpose of propagating constants.

>
>> +void SCCPSolver::visitInsertValueInst(InsertValueInst &IVI) {
>> +  Value *Aggr = IVI.getOperand(0);
> Again, you should use getAggregateOperand().
>> +  Value *Val = IVI.getOperand(1);
> And getInsertedValueOperand() here.
>
>> +  // If the operand to the getresult is an undef, the result is undef.
> That should be insertvalue, not getresult?

fixed.

>
>> +  if (isa<UndefValue>(Aggr))
>> +    return;
>> +
>> +  // Currently only handle single-index insertvalues.
>> +  if (IVI.getNumIndices() != 1) {
>> +    markOverdefined(&IVI);
>> +    return;
>> +  }
>> +
>> +  // See if we are tracking the result of the callee.
>> +  Function *F = IVI.getParent()->getParent();
>> +  std::map<std::pair<Function*, unsigned>, LatticeVal>::iterator
>> +    It = TrackedMultipleRetVals.find(std::make_pair(F,
>> *IVI.idx_begin()));
>> +
>> +  // Merge in the inserted member value.
>> +  if (It != TrackedMultipleRetVals.end())
>> +    mergeInValue(It->second, F, getValueState(Val));
>> +
>> +  // Mark the aggregate result of the IVI overdefined; any tracking
>> that we do will
>> +  // be done on the individual member values.
>> +  markOverdefined(&IVI);
>> +}
> From looking at this code, it seems that you are assuming that all
> insertvalue
> instructions are used to build return values. From the looks of it, I
> could
> fool SCCP into propagting the wrong constant, though I don't really know
> how
> SCPP works.

You're right, good catch. I believe I've fixed this now.

>
>> +      // The aggregate value is used in a way not handled here. Assume
>> nothing.
>> +      markOverdefined(*UI);
> Shouldn't that be return value isntead of aggregate value?

No, it's saying the aggregate value returned by the function has
users that aren't getresult or single-index extractvalue, which
SCCP doesn't attempt to handle.

Dan