[llvm-commits] [llvm] r47607 - in /llvm/trunk: include/llvm/Instructions.h lib/VMCore/Instructions.cpp

[Devang Patel]

On Feb 26, 2008, at 10:26 AM, Chris Lattner wrote:

>> URL: http://llvm.org/viewvc/llvm-project?rev=47607&view=rev
>> Log:
>> Optimize most common case by using single RetVal in ReturnInst.
>
> Nice!
>
>>
>> @@ -1405,6 +1406,23 @@
>>
>>  virtual ReturnInst *clone() const;
>>
>> +  // Transparently provide more efficient getOperand methods.
>> +  Value *getOperand(unsigned i) const {
>> +    assert(i < getNumOperands() && "getOperand() out of range!");
>> +    if (getNumOperands() == 0 || getNumOperands() == 1)
>> +      return RetVal;
>> +
>> +    return OperandList[i];
>> +  }
>> +
>> +  void setOperand(unsigned i, Value *Val) {
>> +    assert(i < getNumOperands() && "setOperand() out of range!");
>> +    if (i == 0)
>> +      RetVal = Val;
>> +    else
>> +      OperandList[i] = Val;
>> +  }
>
> I would suggest just dropping these two methods: it's likely that
> dereferencing OperandList unconditionally is faster or as fast than
> conditional code.

ok

>
>
>
>> ReturnInst::ReturnInst(const ReturnInst &RI)
>>  : TerminatorInst(Type::VoidTy, Instruction::Ret,
>> -                   OperandList, RI.getNumOperands()) {
>> +                   &RetVal, RI.getNumOperands()) {
>>  unsigned N = RI.getNumOperands();
>> +  if (N == 1)
>> +    RetVal.init(RI.RetVal, this);
>> +  else if (N) {
>> +    Use *OL = OperandList = new Use[N];
>> +    for (unsigned i = 0; i < N; ++i)
>> +      OL[i].init(RI.getOperand(i), this);
>> +  }
>
> Nice, please set OperandList to null in the N = 0 case though.  The
> best way to do this is to pass up null instead of &RetVal as the base.

ok

>
>
>> Value *ReturnInst::getReturnValue(unsigned n) const {
>> +  if (getNumOperands() == 0)
>>    return 0;
>
> Is this needed?  calling getReturnValue(0) should assert.
>
>> +  assert (n < getNumOperands() && "getReturnValue out of range!");
>> +  if (getNumOperands() == 1)
>> +    return RetVal;
>> +  else
>> +    return OperandList[n];
>> }
>
> I think this whole method should just be an inline method that just
> does: return getOperand(n);
>
> no?

yup, makes sense.

-
Devang