[llvm-commits] [llvm] r98409 - in /llvm/trunk/lib/Target/ARM: ARMConstantIslandPass.cpp ARMInstrFormats.td ARMInstrInfo.td ARMInstrThumb.td ARMInstrThumb2.td ARMInstrVFP.td ARMLoadStoreOptimizer.cpp AsmPrinter/ARMAsmPrinter.cpp Thumb1InstrInfo.cpp Thumb1RegisterInfo.cpp Thumb2SizeReduction.cpp

[Bob Wilson]

On Mar 13, 2010, at 9:00 AM, Jakob Stoklund Olesen <stoklund at 2pi.dk>  
wrote:

>
> On Mar 12, 2010, at 5:08 PM, Bob Wilson wrote:
>
>> Author: bwilson
>> Date: Fri Mar 12 19:08:20 2010
>> New Revision: 98409
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=98409&view=rev
>> Log:
>> Change ARM ld/st multiple instructions to have variant instructions  
>> for
>> writebacks to the address register.  This gets rid of the hack that  
>> the
>> first register on the list was the magic writeback register  
>> operand.  There
>> was an implicit constraint that if that operand was not reg0 it had  
>> to match
>> the base register operand.  The post-RA scheduler's antidependency  
>> breaker
>> did not understand that constraint and sometimes changed one  
>> without the
>> other.  This also fixes Radar 7495976 and should help the verifier  
>> work
>> better for ARM code.
>>
>> There are now new ld/st instructions explicit writeback operands  
>> and explicit
>> constraints that tie those registers together.
>
> Woot! This was a major blocker for the verifier on ARM.
>
>> Modified: llvm/trunk/lib/Target/ARM/ARMLoadStoreOptimizer.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMLoadStoreOptimizer.cpp?rev=98409&r1=98408&r2=98409&view=diff
>> === 
>> === 
>> === 
>> =====================================================================
>> --- llvm/trunk/lib/Target/ARM/ARMLoadStoreOptimizer.cpp (original)
>> +++ llvm/trunk/lib/Target/ARM/ARMLoadStoreOptimizer.cpp Fri Mar 12  
>> 19:08:20 2010
> ...
>> +  if (!DoMerge)
>> +    return false;
>> +
>> +  unsigned NewOpc = getUpdatingLSMultipleOpcode(Opcode);
>> +  MachineInstrBuilder MIB = BuildMI(MBB, MBBI, dl, TII->get(NewOpc))
>> +    .addReg(Base, getDefRegState(true)) // WB base register
>> +    .addReg(Base, getKillRegState(BaseKill));
>
> For a two-addr instruction, the kill flag on the tied use operand is  
> implied, and it is even invalid to have an explicit kill flag there.
>
> The same issue appears in a couple of spots below.
>
> I would like to get rid of that rule, but for now we are stuck with  
> it.

OK. I didn't see any problems from that but I did't actually try  
running the verifier either.  I was just copying that code from  
elsewhere.  Go ahead and fix it up if you like, or I'll try it later.

> Maybe BaseKill should become <def,dead> on the writeback operand,  
> but in that case there was no need to convert to an *_UPD variant in  
> the first place?

>

It would be really great to avoid the need for extra variant  
instructions but I think there was a reason why the two separate  
operands were needed. I don't know the details, but it is definitely  
something to look into.