| Message ID | 1387293144-11554-18-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 12/17/2013 07:12 AM, Peter Maydell wrote: > +static void disas_cc(DisasContext *s, uint32_t insn) > { > + unsigned int sf, op, y, cond, rn, nzcv, is_imm; > + int label_continue; > + TCGv_i64 tcg_tmp, tcg_y, tcg_rn; Don't you get a may be used uninitialized warning for label_continue? > + tcg_tmp = tcg_temp_new_i64(); > + > + if (cond < 0x0e) { /* not always */ > + int label_match = gen_new_label(); > + label_continue = gen_new_label(); > + arm_gen_test_cc(cond, label_match); > + /* nomatch: */ > + tcg_gen_movi_i64(tcg_tmp, nzcv << 28); > + gen_set_nzcv(tcg_tmp); > + tcg_gen_br(label_continue); > + gen_set_label(label_match); > + } You can't re-use tcg_tmp across basic blocks like this. r~
On 19 December 2013 21:04, Richard Henderson <rth@twiddle.net> wrote: > On 12/17/2013 07:12 AM, Peter Maydell wrote: > >> + tcg_tmp = tcg_temp_new_i64(); >> + >> + if (cond < 0x0e) { /* not always */ >> + int label_match = gen_new_label(); >> + label_continue = gen_new_label(); >> + arm_gen_test_cc(cond, label_match); >> + /* nomatch: */ >> + tcg_gen_movi_i64(tcg_tmp, nzcv << 28); >> + gen_set_nzcv(tcg_tmp); >> + tcg_gen_br(label_continue); >> + gen_set_label(label_match); >> + } > > You can't re-use tcg_tmp across basic blocks like this. Hmm. I clearly don't entirely understand the rules here. The TCG README says "temporaries are only live in a basic block" and "After the end of a basic block, the content of temporaries is destroyed", which I took to mean that the value in the temp was dead after the branch, but that I could freely reuse it for something else afterwards as long as I wrote a new value to it (as we're doing here). I guess that's wrong? thanks -- PMM
On 12/19/2013 01:23 PM, Peter Maydell wrote: > Hmm. I clearly don't entirely understand the rules here. > The TCG README says "temporaries are only live > in a basic block" and "After the end of a basic block, the > content of temporaries is destroyed", which I took to > mean that the value in the temp was dead after the > branch, but that I could freely reuse it for something > else afterwards as long as I wrote a new value to > it (as we're doing here). I guess that's wrong? You need to re-allocate it in each basic block. We don't have enough tcg checking to find errors like this. r~
On 19 December 2013 21:26, Richard Henderson <rth@twiddle.net> wrote: > On 12/19/2013 01:23 PM, Peter Maydell wrote: >> Hmm. I clearly don't entirely understand the rules here. >> The TCG README says "temporaries are only live >> in a basic block" and "After the end of a basic block, the >> content of temporaries is destroyed", which I took to >> mean that the value in the temp was dead after the >> branch, but that I could freely reuse it for something >> else afterwards as long as I wrote a new value to >> it (as we're doing here). I guess that's wrong? > > You need to re-allocate it in each basic block. > > We don't have enough tcg checking to find errors like this. We could probably at least update the README to not be misleading :-) thanks -- PMM
On 19 December 2013 21:04, Richard Henderson <rth@twiddle.net> wrote: > On 12/17/2013 07:12 AM, Peter Maydell wrote: >> +static void disas_cc(DisasContext *s, uint32_t insn) >> { >> + unsigned int sf, op, y, cond, rn, nzcv, is_imm; >> + int label_continue; >> + TCGv_i64 tcg_tmp, tcg_y, tcg_rn; > > Don't you get a may be used uninitialized warning for label_continue? I know some versions of gcc tend to be bad at that, but at least for mine (gcc-4.6.3) it doesn't warn about this even with optimization enabled. Is it worth pulling the assignment label_continue = gen_new_label(); out of the if() to protect against possible less smart compilers? thanks -- PMM
On 12/20/2013 08:19 AM, Peter Maydell wrote: > Is it worth pulling the assignment > label_continue = gen_new_label(); > out of the if() to protect against possible less smart compilers? Sure. Or even initializing to -1. r~
diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c index 4700d6e..3c18d6f 100644 --- a/target-arm/translate-a64.c +++ b/target-arm/translate-a64.c @@ -2483,16 +2483,66 @@ static void disas_adc_sbc(DisasContext *s, uint32_t insn) } } -/* Conditional compare (immediate) */ -static void disas_cc_imm(DisasContext *s, uint32_t insn) +/* C3.5.4 - C3.5.5 Conditional compare (immediate / register) + * 31 30 29 28 27 26 25 24 23 22 21 20 16 15 12 11 10 9 5 4 3 0 + * +--+--+--+------------------------+--------+------+----+--+------+--+-----+ + * |sf|op| S| 1 1 0 1 0 0 1 0 |imm5/rm | cond |i/r |o2| Rn |o3|nzcv | + * +--+--+--+------------------------+--------+------+----+--+------+--+-----+ + * [1] y [0] [0] + */ +static void disas_cc(DisasContext *s, uint32_t insn) { - unsupported_encoding(s, insn); -} + unsigned int sf, op, y, cond, rn, nzcv, is_imm; + int label_continue; + TCGv_i64 tcg_tmp, tcg_y, tcg_rn; -/* Conditional compare (register) */ -static void disas_cc_reg(DisasContext *s, uint32_t insn) -{ - unsupported_encoding(s, insn); + if (!extract32(insn, 29, 1)) { + unallocated_encoding(s); + return; + } + if (insn & (1 << 10 | 1 << 4)) { + unallocated_encoding(s); + return; + } + sf = extract32(insn, 31, 1); + op = extract32(insn, 30, 1); + is_imm = extract32(insn, 11, 1); + y = extract32(insn, 16, 5); /* y = rm (reg) or imm5 (imm) */ + cond = extract32(insn, 12, 4); + rn = extract32(insn, 5, 5); + nzcv = extract32(insn, 0, 4); + + tcg_tmp = tcg_temp_new_i64(); + + if (cond < 0x0e) { /* not always */ + int label_match = gen_new_label(); + label_continue = gen_new_label(); + arm_gen_test_cc(cond, label_match); + /* nomatch: */ + tcg_gen_movi_i64(tcg_tmp, nzcv << 28); + gen_set_nzcv(tcg_tmp); + tcg_gen_br(label_continue); + gen_set_label(label_match); + } + /* match, or condition is always */ + if (is_imm) { + tcg_y = new_tmp_a64(s); + tcg_gen_movi_i64(tcg_y, y); + } else { + tcg_y = cpu_reg(s, y); + } + tcg_rn = cpu_reg(s, rn); + if (op) { + gen_sub_CC(sf, tcg_tmp, tcg_rn, tcg_y); + } else { + gen_add_CC(sf, tcg_tmp, tcg_rn, tcg_y); + } + + tcg_temp_free_i64(tcg_tmp); + + if (cond < 0x0e) { /* continue */ + gen_set_label(label_continue); + } } /* C3.5.6 Conditional select @@ -2846,11 +2896,7 @@ static void disas_data_proc_reg(DisasContext *s, uint32_t insn) disas_adc_sbc(s, insn); break; case 0x2: /* Conditional compare */ - if (insn & (1 << 11)) { /* (immediate) */ - disas_cc_imm(s, insn); - } else { /* (register) */ - disas_cc_reg(s, insn); - } + disas_cc(s, insn); /* both imm and reg forms */ break; case 0x4: /* Conditional select */ disas_cond_select(s, insn);