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

[Chen Gang S]

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