Re: [Qemu-devel] [PATCH] target-tilegx: Execute _start and reach to __libc_start_main successfully

[Chen Gang S <gang.chen@sunrus.com.cn>]

On 2/25/15 00:46, Chris Metcalf wrote:
> On 2/24/2015 11:31 AM, Chen Gang S wrote:
>>   - When dst is zero, in most cases, we can just skip the insn, but in
>>     some cases, it may cause exception in user mode (e.g st zero r0).
> 
> Be careful of skipping instructions in general, since for example a "ld zero, r1" may be targeting an MMIO address that we are reading to trigger some device operation, but don't need the result from.

OK, thanks, I shall notice about it, next.

> 
>> - For add/addi operation, it will change to mov/movi operation.
>>
>>   - For mov operation, it will change to movi operation.
>>
>>   - For shl3add, if srcb is zero register, it will change to shli
>>     operation.
> 
> I assume you are referring to missed performance optimization opportunities if you don't treat "zero" specially?  You certainly could treat "add r1, r2, zero" as just an "add" instruction anyway, just with a zero addend.  You don't have to convert it to a move instruction.  Likewise with the others.
> 

OK, thanks. Next, I shall not change the instruction, and will only use
tcg temporary variable instead of zero register, when I send patch v2.

>> OK, thanks.  originally I wanted to reuse them, but after think of, I
>> prefer the 64-bit immediate number instead of.
>>
>>   - The decoding function may be very long, but it is still a simple
>>     function, I guess, it is still simple enough for readers.
>>
>>   - 64-bit immediate number has better performance under 64-bit machine
>>     (e.g x86-64).
> 
> What I mean is you should just directly use all those accessor functions.  Then your code would look like
> 
> switch (get_Opcode_X1(bundle)) {   // this is a 59-bit shift and mask by 0x7
> case SHL16INSLI_OPCODE_X1:   // this is the constant 7
>   [...]
> }
> 
> Typically dealing with smaller integers is higher-performance on any platform, I suspect, even on x86 when you can have large inline constants in the code.  The compiler might be smart enough to do this for you, to be fair.  In any case any performance difference of this switch is almost certainly lost in the noise.
> 

Hmm... maybe what you said is correct, but I am not quite sure.

> More to the point, named constants are almost always better for code maintainability than raw integers in code.
> 

For me, if the raw integer is only used once, we needn't define a macro
for it (instead of, we can give a comment for it).

> Also, my point is that you should just include a verbatim copy of the kernel header into qemu, and then use those names.  This is helpful to people who are already familiar with the <arch/opcode.h> naming, and reduces the amount of code needed to review qemu in any case.
> 

OK, thanks. What you said above sounds reasonable to me, for compatible
reason, I shall use "arch/opcode_tilegx.h" totally -- it will be helpful
for the readers who are already familiar with "arch/opcode_tilegx.h".

Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed