[llvm-commits] [llvm] r57258 - in /llvm/trunk/lib/Target/ARM: ARMCodeEmitter.cpp ARMInstrInfo.h

[Jim Grosbach]

On Oct 7, 2008, at 11:36 PM, Chris Lattner wrote:
>
>> +++ llvm/trunk/lib/Target/ARM/ARMCodeEmitter.cpp Tue Oct  7  
>> 14:05:35 2008
>> @@ -256,8 +256,8 @@
>> unsigned ARMCodeEmitter::getAddrModeNoneInstrBinary(const  
>> MachineInstr &MI,
>>                                                    const  
>> TargetInstrDesc &TID,
>>                                                    unsigned Binary) {
>> -  // FIXME: Assume CC is AL for now.
>> -  Binary |= ARMCC::AL << 28;
>> +  // Set the conditional execution predicate
>> +  Binary |= II->getPredicate(&MI) << 28;
>
> This makes the assumption that the ARMCC::CondCodes enums follows  
> exactly with the encodings used by the ARM instruction set.  THis is  
> a reasonable thing, but is a new requirement.  Please put a comment  
> above the CondCodes enum that points out that the numbering *must*  
> follow the ISA.


A comment to that effect is good sense. I'll add it. I wasn't  
(intentionally) adding a new requirement. My understanding was that  
the equivalent was already asserted, as evidenced by, for example, the  
"ARMCC::A: << 28" that the new code is replacing. That doesn't really  
matter, though. A comment is a good idea regardless. Done.

-Jim