[llvm-commits] [llvm] r72631 - in /llvm/trunk: include/llvm/CodeGen/ include/llvm/Target/ lib/CodeGen/ lib/ExecutionEngine/JIT/ lib/Target/ARM/ lib/Target/Alpha/ lib/Target/PowerPC/ lib/Target/X86/

[Bruno Cardoso Lopes]

Hi Anton,

On Sat, May 30, 2009 at 6:15 PM, Anton Korobeynikov
<anton at korobeynikov.info> wrote:
> Hello, Bruno
>
>> First patch in the direction of splitting MachineCodeEmitter in two subclasses:
>> JITCodeEmitter and ObjectCodeEmitter. No functional changes yet. Patch by Aaron Gray
> Sorry, I was not able to look into it before

Thanks for taking a look! :)

>> +  void emitWordLE(unsigned W) {
>> +    if (4 <= BufferEnd-CurBufferPtr) {
>> +      *CurBufferPtr++ = (unsigned char)(W >>  0);
>> +      *CurBufferPtr++ = (unsigned char)(W >>  8);
>> +      *CurBufferPtr++ = (unsigned char)(W >> 16);
>> +      *CurBufferPtr++ = (unsigned char)(W >> 24);
>> +    } else {
>> +      CurBufferPtr = BufferEnd;
>> +    }
>> +  }
> Please use uint8_t here and everywhere instead of unsigned char.
>
>> +  /// emitInt32 - Emit a int32 directive.
>> +  void emitInt32(int Value) {
> Please use int32_t here. Same for other places.

We actually left those in the same way they are now in MachineCodeEmitter.h
But for sure this can be improved in a new patch

>
>> +
>> +FunctionPass *createARMCodeEmitterPass(
>> +    ARMTargetMachine &TM, MachineCodeEmitter &MCE);
>> +/*
>> +template< class machineCodeEmitter>
>> +FunctionPass *createARMCodeEmitterPass(
>> +    ARMTargetMachine &TM, machineCodeEmitter &MCE);
>> +*/
> Huh? Also, please follow LLVM Coding Style convention.
>
>> +
>> +  template< class machineCodeEmitter>
> No space before "class". Maybe it's better to select another name for
> template? MachineCodeEmitter vs machineCodeEmitter looks ambiguous.
>
>> +  class VISIBILITY_HIDDEN Emitter : public MachineFunctionPass,
>> +     public ARMCodeEmitter
>> +  {
> Here also. "{" is normally put on the same line as class def.
>
>> +
>> +namespace llvm {
>> +
>> +FunctionPass *createARMCodeEmitterPass(
>> +    ARMTargetMachine &TM, MachineCodeEmitter &MCE)
>> +{
>> +  return new Emitter<MachineCodeEmitter>(TM, MCE);
>> +}
>> +FunctionPass *createARMJITCodeEmitterPass(
>> +    ARMTargetMachine &TM, JITCodeEmitter &JCE)
>> +{
>> +  return new Emitter<JITCodeEmitter>(TM, JCE);
>>  }
> See above about coding style.
>
>> +  template <class machineCodeEmitter>
>> +  class VISIBILITY_HIDDEN Emitter : public MachineFunctionPass,
>> +      public AlphaCodeEmitter
>> +  {
> Same here
>
>> +
>> +FunctionPass *llvm::createAlphaCodeEmitterPass( AlphaTargetMachine &TM,
>>                                                 MachineCodeEmitter &MCE) {
>> -  return new AlphaCodeEmitter(TM, MCE);
>> +  return new Emitter<MachineCodeEmitter>(TM, MCE);
>> +}
> And here...
>
>> +
>> +FunctionPass *llvm::createAlphaJITCodeEmitterPass( AlphaTargetMachine &TM,
>> +                                               JITCodeEmitter &JCE) {
>> +  return new Emitter<JITCodeEmitter>(TM, JCE);
>>  }
> And here.
>
>> +    /// getBinaryCodeForInstr - This function, generated by the
>> +    /// CodeEmitterGenerator using TableGen, produces the binary encoding for
>> +    /// machine instructions.
>> +
>> +    unsigned getBinaryCodeForInstr(const MachineInstr &MI);
> Do not add blank lines after comment blocks.
>
> <same comments applies for stuff later>

Agreed on coding style.

-- 
Bruno Cardoso Lopes
http://www.brunocardoso.cc