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

[Chen Gang S]

On 02/26/2015 01:19 AM, Richard Henderson wrote:
> On 02/24/2015 05:40 PM, Chen Gang S wrote:
>>>> +static int gen_shl16insli(struct DisasContext *dc,
>>>> +                          unsigned char rdst, unsigned char rsrc, short 
>>>> im16)
>>>
>>> Use uint16_t, as the field is not signed.
>>>
>>
>> objdump print it as signed, e.g. "shl16insli r32, r32, -30680"
> 
> I think you've just found a bug in objdump.  ;-)
> 

According to the ISA document, I guess, it assumes imm16 is unsigned
number (although ISA document does not mention about it, directly). But
for tcg_gen_ori_i64, it assumes imm16 is signed number:

  void tcg_gen_ori_i64(TCGv_i64 ret, TCGv_i64 arg1, int64_t arg2);

So for me, I am not sure that objdump must be a bug. For safety reason,
I still prefer to treat imm16 as signed number.

>>> Use the extract64 and sextract64 functions instead of bare shifts and masks.
>>>
>>
>> OK, thanks What you said above sounds reasonable to me. But if we use
>> "arch/opcode_tilegx.h" next, we can use their own related inline
>> functions instead of.
> 
> Yes, using the opcode_tilegx.h functions is a good idea.  I didn't know about
> those before.  I've looked through that file now and using them will make
> reviewing this code much easier.
> 

OK, thanks.

>>>> +    /* For can regress completely, it must be y0 -> y1 -> y2, or x0 -> 
>>>> x1. */
>>>
>>> I don't know what this sentence means.
>>>
>>
>> I guess, it needs more contents:
>>
>>   y2 and x1 may contents st instruction (x1 may also contents 'jal'),
>>   which should be executed at last, or when exception raised, we can
>>   not be sure each bundle must be atomic (none pipes should execute).
> 
> How about something like:
> 
>       Only pipelines X1 and Y2 can issue branches, memory ops, or
>       issue trapping instructions like system calls.  As these

Y1 can issue branches, which can be buffered. And Y1 also can issue call
(which will store pc to sp stack) -- e.g. jalr, jalrp.

>       operations cannot be buffered, it is convenient to translate
>       those pipelines last.
> 
> After all, we're already buffering the output of all of the pipes.  If the
> memory op succeeds, then all of the rest of the arithmetic will succeed, so it
> doesn't really matter that much what ordering we give the pipes.  No state 
> will
> change until we reach the end of the bundle and write back the results.
> 

OK, thanks. After check ISA document again, for me, we have to still use
"y0, y1, y2", e.g. de5e598034ac3000 { fnop ; jalr r12 ; st r10, r11 }

 If y0 -> y1 -> y2:

 - if jalr succeeds, it will write pc to sp stack, but sp is not changed
   (just like lr, pc, they are buffered to tcg temporary variables).

 - if st fails, as the result, we can still say the whole bundle is not
   execute (it has already written pc to sp stack, but sp isn't changed,
   so it is still OK).

 If y0 -> y2 -> y1:

 - if st succeeds, it will write data to the useful memory.

 - if jalr fails (e.g. sp stack is full, which may cause memory access
   issue), we can not restore the bundle.


If what I said is correct, for me, we still need some comments, but the
comments really need to be improved, it may like this:

  Must be sure of pipe y1 before pipe y2, or the bundle { ; jalr ... ;
  st ...} may not be restored if failure occurs. For common habbit, we
  can still use "y0, y1, y2" and "x0, x1", it is OK (y1 before y2).
  


>>> You don't need these gotos.  If a given pipeline raises an invalid 
>>> instruction
>>> exception, then it's true that all code emitted afterward is dead.  But 
>>> that's
>>> ok, since it simply won't be executed.  Invalid instructions are exceedingly
>>> unlikely and it's silly to optimize for that case.
>>>
>>
>> OK, thanks. I guess your meaning is: we need not check the return value
>> for each pipe, just raise exception is enough. e.g.
>>
>>  - Add a flag in DisasContext.
>>
>>  - When exception happens, set the flag.
>>
>>  - In main looping gen_intermediate_code_internal(), if the flag is set,
>>    we need break the tb block, and return.
> 
> Yes, that will be fine.

OK, thanks.


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