Re: [Qemu-devel] [PATCH 03/15] Openrisc: add instructions translation

[Max Filippov]

Hi Jia.

>>> +    case 0x0009:
>>> +        switch (op1) {
>>> +        case 0x03:   /*l.div*/
>>> +            LOG_DIS("l.div r%d, r%d, r%d\n", rd, ra, rb);
>>> +            if (rb != 0) {
>>> +                tcg_gen_div_tl(cpu_R[rd], cpu_R[ra], cpu_R[rb]);
>>
>> You also need to take care of integer overflow/division by zero here,
>> otherwise it may crash QEMU.
>>
>>> +            } else {
>>> +                /* exception here */
>
> overflow/division by zero is handled here by patch 05/15.

In that patch I have found only exception generation in the 'else' branch.
However code generated by tcg_gen_div_tl in the 'if' branch will crash QEMU
in case rb register carries zero or ra/rb gives integer overflow.

>>> +    case 0x000b:
>>> +        switch (op1) {
>>> +        case 0x03:   /*l.mulu*/
>>> +            LOG_DIS("l.mulu r%d, r%d, r%d\n", rd, ra, rb);
>>> +            if (rb != 0 && ra != 0) {
>>> +                tcg_gen_ext32u_tl(cpu_R[ra], cpu_R[ra]);
>>> +                tcg_gen_ext32u_tl(cpu_R[rb], cpu_R[rb]);
>>
>> This code would clobber high 32 bits of ra and rb if it was compiled
>> for 64 bit registers.
>> Are you going to support both 32 and 64 bit version of the ISA?
>> Could you please clarify *_tl usage here?
>>
>
> I used uint32_t at first, change it into *_tl for 64bit support in the future.
> May I keep it *_tl? If I can not, I'll back it to uint32.

Ok, to me *_tl looks pretty reasonable for instructions that scale uniformly
with register width, like simple l.and/l.or/l.add/l.div/etc.

For instructions that has less symmetry regarding register width or that
operate on registers of different widths at once, like l.maci, I'd say that
explicit *_i32/*_i64 would be more informative.

For this particular mulu I guess the correct implementation, suitable for both
32 and 64 bit wide registers would be just

    tcg_gen_mul_tl(cpu_R[rd], cpu_R[ra], cpu_R[rb]);

however it doesn't take care of SR[OV], SR[CY]. I haven't found a place
where you update these bits.

>>> +static void dec_float(DisasContext *dc, CPUOPENRISCState *env, uint32_t 
>>> insn)
>>> +{
>>> +    uint32_t op0;
>>> +    uint32_t ra, rb, rd;
>>> +    op0 = field(insn, 0, 8);
>>> +    ra = field(insn, 16, 5);
>>> +    rb = field(insn, 11, 5);
>>> +    rd = field(insn, 21, 5);
>>> +
>>> +    switch (op0) {
>>> +    case 0x10:    /*lf.add.d*/
>>> +        LOG_DIS("lf.add.d r%d, r%d, r%d\n", rd, ra, rb);
>>> +        tcg_gen_add_i64(cpu_R[rd], cpu_R[ra], cpu_R[rb]);
>>
>> Through this function you generate integer operations on the
>> registers, although ISA
>> suggests that there should be either single- or double-precision
>> floating point operations.
>>
>
> Sorry, I  didn't find a TCG-IR that make single- or double-precision
> floating point operations, may you give me some hits?

As chenwj said it is usually done in helper functions.
Software floating point structures, prototypes and implementations
live in the fpu/ directory.

Good example of how to use them is MIPS:
target-mips/translate.c, function gen_farith and
target-mips/op_helper.c, macro FLOAT_BINOP

-- 
Thanks.
-- Max