[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

[Chris Lattner]

On Apr 28, 2008, at 11:41 AM, Dan Gohman wrote:
>>> /// 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.

Thanks!
>>> +    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.

True: the direction I was trying to go is to make CreateRegForValue a  
private method in RegsForValue.  This might not make sense, but would  
reduce one concept out of SDISel.

>>> @@ -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 :-).

Please do, it is much more idiomatic to use push_back and the vector  
was clearly empty before.

>>> // 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.

Ok.

>>> @@ -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.

Ok.  Thanks Dan,

-Chris