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

[Chris Lattner]

On Oct 8, 2008, at 9:28 AM, Jim Grosbach wrote:

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

Good point, thanks Jim!

-Chris