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

[Bob Wilson]

On Sep 3, 2010, at 3:38 PM, Jakob Stoklund Olesen wrote:

> 
> 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++));

OK, thanks for suggesting that.  I've changed all of those.

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

The double-spaced VLD3/VLD4 instructions will have an explicit source operand for the super register.  (It is tied to the destination operand, so it will always be the same register at this point.)  The operand is basically being transferred from an explicit operand on the pseudo instruction to an implicit operand on the real instruction.  Because of that, we shouldn't have to worry about conflicts with arbitrary implicit operands.

As we discussed Friday, I also added the imp-use for both of the "even" and "odd" VLDs.

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