[llvm-commits] [llvm] r112983 - in /llvm/trunk/lib/Target/ARM: ARMExpandPseudoInsts.cpp ARMISelDAGToDAG.cpp ARMInstrNEON.td NEONPreAllocPass.cpp

[Jakob Stoklund Olesen]

On Sep 3, 2010, at 11:16 AM, Bob Wilson wrote:

> Author: bwilson
> Date: Fri Sep  3 13:16:02 2010
> New Revision: 112983
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=112983&view=rev
> Log:
> Finish converting the rest of the NEON VLD instructions to use pseudo-
> instructions prior to regalloc.  Since it's getting a little close to
> the 2.8 branch deadline, I'll have to leave the rest of the instructions
> handled by the NEONPreAllocPass for now, but I didn't want to leave half
> of the VLD instructions converted and the other half not.
> 
> Modified:
>    llvm/trunk/lib/Target/ARM/ARMExpandPseudoInsts.cpp
>    llvm/trunk/lib/Target/ARM/ARMISelDAGToDAG.cpp
>    llvm/trunk/lib/Target/ARM/ARMInstrNEON.td
>    llvm/trunk/lib/Target/ARM/NEONPreAllocPass.cpp
> 
> Modified: llvm/trunk/lib/Target/ARM/ARMExpandPseudoInsts.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMExpandPseudoInsts.cpp?rev=112983&r1=112982&r2=112983&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/ARM/ARMExpandPseudoInsts.cpp (original)
> +++ llvm/trunk/lib/Target/ARM/ARMExpandPseudoInsts.cpp Fri Sep  3 13:16:02 2010
> @@ -105,16 +105,17 @@
>     D2 = TRI->getSubReg(DstReg, ARM::dsub_5);
>     D3 = TRI->getSubReg(DstReg, ARM::dsub_7);
>   } 
> -  MIB.addReg(D0).addReg(D1);
> +  MIB.addReg(D0, RegState::Define | getDeadRegState(DstIsDead))
> +    .addReg(D1, RegState::Define | getDeadRegState(DstIsDead));
>   if (NumRegs > 2)
> -    MIB.addReg(D2);
> +    MIB.addReg(D2, RegState::Define | getDeadRegState(DstIsDead));
>   if (NumRegs > 3)
> -    MIB.addReg(D3);
> +    MIB.addReg(D3, RegState::Define | getDeadRegState(DstIsDead));

Thanks!

> -    MIB.addReg(WBReg, getDefRegState(true) | getDeadRegState(WBIsDead));
> +    MIB.addReg(WBReg, RegState::Define | getDeadRegState(WBIsDead));

Right. I think using the enums directly is fine. The functions were added as helpers when your <dead> bit is variable like here.

Not in this patch, but related:

  // Copy the addrmode6 operands.
  bool AddrIsKill = MI.getOperand(OpIdx).isKill();
  MIB.addReg(MI.getOperand(OpIdx++).getReg(), getKillRegState(AddrIsKill));
  MIB.addImm(MI.getOperand(OpIdx++).getImm());

Is easier like this:

  MIB.addOperand(MI.getOperand(OpIdx++));
  MIB.addOperand(MI.getOperand(OpIdx++));


>   }
>   // Copy the addrmode6 operands.
>   bool AddrIsKill = MI.getOperand(OpIdx).isKill();
> @@ -128,9 +129,12 @@
> 
>   MIB = AddDefaultPred(MIB);
>   TransferImpOps(MI, MIB, MIB);
> -  // Add an implicit def for the super-reg.
> -  MIB.addReg(DstReg, (getDefRegState(true) | getDeadRegState(DstIsDead) |
> -                      getImplRegState(true)));
> +  // For an instruction writing the odd subregs, add an implicit use of the
> +  // super-register because the even subregs were loaded separately.
> +  if (RegSpc == OddDblSpc)
> +    MIB.addReg(DstReg, RegState::Implicit);

This part is weird. Is OddDblSpc guaranteed to have DstReg live in? In that case, would there already be a DstReg<imp-use> operand on MI that gets transferred by TransferImpOps()?

The end result should be:

%D0, %D2, ... = VLD ..., %QQQQ0<imp-def>
%D1, %D3, ... = VLD ..., %QQQQ0<imp-use,kill>, %QQQQ0<imp-def>


> +  // Add an implicit def for the super-register.
> +  MIB.addReg(DstReg, RegState::ImplicitDefine | getDeadRegState(DstIsDead));

This is correct, but since TransferImpOps() has added arbitrary imp-ops to your new instruction, it would be better to call addRegisterDefined() / addRegisterDead(). That handles existing operands, and it will even flip the <dead> bit on subregs if you pass in a TRI.

You could argue that we are missing a MachineInstr::addRegisterUsed() function to complete the quartet.

/jakob