Re: [Qemu-devel] [PATCH v8 08/16] target-or32: Add instruction translation

[Jia Liu <proljc@gmail.com>]

Hi Blue,

On Fri, Jul 6, 2012 at 2:45 AM, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Wed, Jul 4, 2012 at 11:59 PM, Jia Liu <proljc@gmail.com> wrote:
>> On Wed, Jul 4, 2012 at 10:23 AM, Jia Liu <proljc@gmail.com> wrote:
>>> Hi Blue,
>>>
>>> On Wed, Jul 4, 2012 at 2:48 AM, Blue Swirl <blauwirbel@gmail.com> wrote:
>>>> On Sun, Jul 1, 2012 at 2:45 AM, Jia Liu <proljc@gmail.com> wrote:
>>>>> +    switch (op0) {
>>>>> +    case 0x00:    /* l.j */
>>>>> +        LOG_DIS("l.j %d\n", N26);
>>>>> +    case 0x01:    /* l.jal */
>>>>> +        LOG_DIS("l.jal %d\n", N26);
>>>>> +    case 0x03:    /* l.bnf */
>>>>> +        LOG_DIS("l.bnf %d\n", N26);
>>>>> +    case 0x04:    /* l.bf */
>>>>> +        LOG_DIS("l.bf %d\n", N26);
>>>>> +
>>>>> +        gen_jump(dc, N26, 0, op0);
>>>>
>>>> The cases 0x00 to 0x03 will fall through to this case, calling LOG_DIS
>>>> one to four times.
>>>
>>> Sorry, I can't find a better path. Is this code OK?
>>>
>>>     switch (op0) {
>>>     case 0x00:    /* l.j */
>>>     case 0x01:    /* l.jal */
>>>     case 0x03:    /* l.bnf */
>>>     case 0x04:    /* l.bf */
>>>         LOG_DIS("l.j/l.jal/l.bnf/l.bf %d\n", N26);
>>>
>>>         gen_jump(dc, N26, 0, op0);
>>>
>>
>> or is this code OK?
>>
>>     switch (op0) {
>>     case 0x00:    /* l.j */
>>         LOG_DIS("l.j %d\n", N26);
>>         gen_jump(dc, N26, 0, op0);
>>         break;
>>     case 0x01:    /* l.jal */
>>         LOG_DIS("l.jal %d\n", N26);
>>         gen_jump(dc, N26, 0, op0);
>>         break;
>>     case 0x03:    /* l.bnf */
>>         LOG_DIS("l.bnf %d\n", N26);
>>         gen_jump(dc, N26, 0, op0);
>>         break;
>>     case 0x04:    /* l.bf */
>>         LOG_DIS("l.bf %d\n", N26);
>>         gen_jump(dc, N26, 0, op0);
>>         break;
>
> Yes, with N26 defined only when LOG_DIS does something (see below).
>
>>
>>
>>>>> +
>>>>> +    case 0x11:    /* l.jr */
>>>>> +        LOG_DIS("l.jr r%d\n", rb);
>>>>> +    case 0x12:    /* l.jalr */
>>>>> +        LOG_DIS("l.jalr r%d\n", rb);
>>>>> +
>>>>> +        gen_jump(dc, 0, rb, op0);
>>>>
>>>> Also here.
>>>>
>>>
>>> And, this?
>>>
>>      case 0x11:    /* l.jr */
>
> Now there's no LOG_DIS() line.

Oh! I was too careless! It shouldn't a LOG_DIS() here!
Thank you very much!

>
>>          gen_jump(dc, 0, rb, op0);
>>          break;
>>      case 0x12:    /* l.jalr */
>>          LOG_DIS("l.jr r%d\n", rb);
>>          gen_jump(dc, 0, rb, op0);
>>          break;
>>
>>>>> +static void dec_sys(DisasContext *dc, uint32_t insn)
>>>>> +{
>>>>> +    uint32_t op0;
>>>>> +    /*uint32_t K16;*/
>>>>> +    op0 = field(insn, 16, 8);
>>>>> +    /*K16 = field(insn, 0, 16);*/
>>>>> +
>>>>> +    switch (op0) {
>>>>> +    case 0x000:    /* l.sys */
>>>>> +        /*LOG_DIS("l.sys %d\n", K16);*/
>>>>
>>>> Why commented out?
>>>
>>> When I build QEMU, I got a error msg like "unused var K16, treat
>>> warnings as errors".
>>> I didn't find a better way to handle it, so, commented it.
>
> This can be avoided by adjusting those variable definitions which are
> only used by LOG_DIS() so that they are only defined when LOG_DIS
> needs them:
> #ifdef OPENRISC_DISAS
>   int K16, N26;
> #endif
>

Thank you very much! This is a very nice way.


>>>
>>>>
>>>>> +        tcg_gen_movi_tl(cpu_pc, dc->pc);
>>>>> +        gen_exception(dc, EXCP_SYSCALL);
>>>>> +        dc->is_jmp = DISAS_UPDATE;
>>>>> +        break;
>>>>> +
>>>>> +    case 0x100:    /* l.trap */
>>>>> +        /*LOG_DIS("l.trap %d\n", K16);*/
>>>>
>>>> Ditto
>>>>
>>>
>>> And, commented it here.
>>>
>>>>> +        tcg_gen_movi_tl(cpu_pc, dc->pc);
>>>>> +        gen_exception(dc, EXCP_TRAP);
>>>>> +        break;
>>>>> +
>>>
>>> Regards,
>>> Jia.
>>
>>
>> Regards,
>> Jia.


Regards,
Jia.