[llvm-commits] [llvm] r50279 - in /llvm/trunk: lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp lib/Transforms/Scalar/CodeGenPrepare.cpp test/CodeGen/Generic/multiple-return-values-cross-block-with-invoke.ll

[Dan Gohman]

On Apr 28, 2008, at 12:13 AM, Chris Lattner wrote:

> On Apr 25, 2008, at 11:27 AM, Dan Gohman wrote:
>>
>>  /// RegsForValue - This struct represents the physical registers
>> that a
>>  /// particular value is assigned and the type information about
>> the value.
>>  /// This is needed because values can be promoted into larger
>> registers and
>>  /// expanded into multiple smaller registers than the value.
>>  struct VISIBILITY_HIDDEN RegsForValue {
>
> This should probably explicitly mention how it handles aggregates.
> Maybe a good general summary up front could help my larger complaint
> next:

I rewrote this comment. Let me know if have further questions.

>
>
>
> ...
>
>> +    /// RegVTs - The value types of the registers. This is the same
>> size
>> +    /// as ValueVTs; every register contributing to a given value
>> must
>> +    /// have the same type. When Regs contains all virtual
>> registers, the
>> +    /// contents of RegVTs is redundant with TLI's getRegisterType
>> member
>> +    /// function, however when Regs contains physical registers, it
>> is
>> +    /// necessary to have a separate record of the types.
>>    ///
>> +    SmallVector<MVT::ValueType, 4> RegVTs;
>>
>> +    /// ValueVTs - The value types of the values, which may be
>> promoted
>> +    /// or synthesized from one or more registers.
>> +    SmallVector<MVT::ValueType, 4> ValueVTs;
>
> The descriptions of these fields is not very clear to me.  The
> description of RegVTs forward references ValueVTs, maybe ValueVTs
> should be moved first?  It would be helpful to have an explanatory
> comment here with an example.  For example, on PPC, how is {i8, i64}
> handled by these two vectors?  Maybe 'Regs' should have a comment that
> indicates it can contain pregs?

I rewrote several related comments to be more explanatory.

>
>
>>
>> +
>> +    RegsForValue() : TLI(0) {}
>> +
>> +    RegsForValue(const TargetLowering &tli,
>> +                 unsigned Reg, MVT::ValueType regvt, MVT::ValueType
>> valuevt)
>> +      : TLI(&tli), Regs(1, Reg), RegVTs(1, regvt), ValueVTs(1,
>> valuevt) {}
>> +    RegsForValue(const TargetLowering &tli,
>> +                 const std::vector<unsigned> &regs,
>>                 MVT::ValueType regvt, MVT::ValueType valuevt)
>> +      : TLI(&tli), Regs(regs), RegVTs(1, regvt), ValueVTs(1,
>> valuevt) {}
>> +    RegsForValue(const TargetLowering &tli,
>> +                 const std::vector<unsigned> &regs,
>> +                 const SmallVector<MVT::ValueType, 4> &regvts,
>> +                 const SmallVector<MVT::ValueType, 4> &valuevts)
>> +      : TLI(&tli), Regs(regs), RegVTs(regvts), ValueVTs(valuevts) {}
>> +    RegsForValue(const TargetLowering &tli,
>> +                 unsigned Reg, const Type *Ty) : TLI(&tli) {
>
> So many ctors... would it be reasonable to change clients to use one
> of the more general ones instead of having so many ctors?

One of them was unused; I deleted it.

>
>
>>
>> +      ComputeValueVTs(tli, Ty, ValueVTs);
>> +
>> +      for (unsigned Value = 0; Value != ValueVTs.size(); ++Value) {
>
> Please add a comment explaining what this loop is doing.  Does
> 'ValueVTs.size()' need to be recomputed on every iteration?

No, it ought to be inlined :-). I fixed it though.

>
>
>>
>> +        MVT::ValueType ValueVT = ValueVTs[Value];
>> +        unsigned NumRegs = TLI->getNumRegisters(ValueVT);
>> +        MVT::ValueType RegisterVT = TLI->getRegisterType(ValueVT);
>> +        for (unsigned i = 0; i != NumRegs; ++i)
>> +          Regs.push_back(Reg + i);
>> +        RegVTs.push_back(RegisterVT);
>> +        Reg += NumRegs;
>> +      }
>>    }
>
>
>> @@ -310,16 +362,22 @@
>> /// the correctly promoted or expanded types.  Assign these registers
>> /// consecutive vreg numbers and return the first assigned number.
>> unsigned FunctionLoweringInfo::CreateRegForValue(const Value *V) {
>> +  const Type *Ty = V->getType();
>> +  SmallVector<MVT::ValueType, 4> ValueVTs;
>> +  ComputeValueVTs(TLI, Ty, ValueVTs);
>> +
>> +  unsigned FirstReg = 0;
>> +  for (unsigned Value = 0; Value != ValueVTs.size(); ++Value) {
>> +    MVT::ValueType ValueVT = ValueVTs[Value];
>> +    unsigned NumRegs = TLI.getNumRegisters(ValueVT);
>> +    MVT::ValueType RegisterVT = TLI.getRegisterType(ValueVT);
>>
>> +    for (unsigned i = 0; i != NumRegs; ++i) {
>> +      unsigned R = MakeReg(RegisterVT);
>> +      if (!FirstReg) FirstReg = R;
>> +    }
>> +  }
>> +  return FirstReg;
>> }
>
> The comments in this method should be improved to talk about what it
> is now doing.  Please mention aggregates.  Could this code change to
> use RegsForValue to compute the expansion?  Maybe it should be a new
> method on RegsForValue?

I added a comment that mentions aggregates. This code does
use ComputeValueVTs to do most of the work; that's a function
I factored out specifically for the purpose of using it
in both CreateRegForValue and RegsForValue.

>
>
>> @@ -3898,8 +3985,9 @@
>>        if ((NumOps & 7) == 2 /*REGDEF*/) {
>>          // Add NumOps>>3 registers to MatchedRegs.
>>          RegsForValue MatchedRegs;
>> -          MatchedRegs.ValueVT = InOperandVal.getValueType();
>> -          MatchedRegs.RegVT   = AsmNodeOperands[CurOp
>> +1].getValueType();
>> +          MatchedRegs.TLI = &TLI;
>> +          MatchedRegs.ValueVTs.resize(1,
>> InOperandVal.getValueType());
>> +          MatchedRegs.RegVTs.resize(1, AsmNodeOperands[CurOp
>> +1].getValueType());
>
> Please use push_back, instead of resize(1, x)

I used resize because it's initializing the vector contents
with exactly one element. But I'll change it if you insist :-).

>
>
>> @@ -4304,21 +4392,14 @@
>>  }
>>
>>  // Figure out the result value types. We start by making a list of
>> +  // the potentially illegal return value types.
>>  SmallVector<MVT::ValueType, 4> LoweredRetTys;
>>  SmallVector<MVT::ValueType, 4> RetTys;
>> +  ComputeValueVTs(*this, RetTy, RetTys);
>>
>> +  // Then we translate that to a list of legal types.
>> +  for (unsigned I = 0, E = RetTys.size(); I != E; ++I) {
>> +    MVT::ValueType VT = RetTys[I];
>>    MVT::ValueType RegisterVT = getRegisterType(VT);
>>    unsigned NumRegs = getNumRegisters(VT);
>>    for (unsigned i = 0; i != NumRegs; ++i)
>
> This seems like something that should be a method on RegsForValue.

It isn't trivial to do though. I expect we'll continue to
refactor RegsForValue and eventually get this cleaned up.

>
>
>> @@ -4441,19 +4522,11 @@
>>          cast<RegisterSDNode>(Op.getOperand(1))->getReg() != Reg) &&
>>         "Copy from a reg to the same reg!");
>>  assert(!TargetRegisterInfo::isPhysicalRegister(Reg) && "Is a
>> physreg");
>> +  RegsForValue RFV(TLI, Reg, V->getType());
>> +  SDOperand Chain = DAG.getEntryNode();
>> +  RFV.getCopyToRegs(Op, DAG, Chain, 0);
>> +  PendingExports.push_back(Chain);
>> }
>
> I'm not sure if your patch is responsible, but it is very strange to
> me that RegsForValue::getCopyToRegs takes 'Chain' byref and mutates
> the input.  Should RegsForValue::getCopyToRegs return a new chain
> instead?

Perhaps, but currently the Chain handling is consistent with
the optional Flag handling.

Dan