[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 25, 2008, at 11:27 AM, Dan Gohman wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=50279&view=rev
> Log:
> Remove the code from CodeGenPrepare that moved getresult instructions
> to the block that defines their operands. This doesn't work in the
> case that the operand is an invoke, because invoke is a terminator
> and must be the last instruction in a block.

Nice!  Thanks for tackling this.

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


...

> +    /// 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?

>
> +
> +    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?

>
> +      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?

>
> +        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?

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

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

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

Thanks again for doing working on this Dan!

-Chris