On 6/3/15 00:32, Richard Henderson wrote: > On 06/01/2015 01:54 PM, Chen Gang wrote: >>> Further, the < TILEGX_R_COUNT restriction is also incorrect. True, you >>> don't >>> actually implement the top 7 special registers, but that doesn't matter, you >>> should still be incrementing them. >>> >> >> We did not implement them, so can not increment them, either. >> >> They are hidden to outside, or we have to define and implement them. >> >> So for me, the current code is correct. > > It isn't correct, it's simply functional. These registers may eventually be > implemented, and at that point this code will fail. You'll note that your > store_add functions don't have the same problem, because they don't have this > R_COUNT check. It would be better to increase the number of buffer slots and > do the right thing here in load_add. >
For me, it is about 2 discussions: - Whether need implement additional 7 registers. I guess not. But if we will really implement them in future, we need only let TILEGX_R_COUNT = TILEGX_R_ZERO, and all things should still be OK. - Whether need 2 or more tmp variables for one pipe. It is not necessary, but it will let the code simplier. > My suggestion is to expand tmp_regs to 4, drop tmp_regcur, and have dest_gr > manage all of the indexing. I.e. > > static TCGv dest_gr(DisasContext *dc, uint8_t rdst) > { > int n = dc->n_tmp_regs++; > assert(n < ARRAY_SIZE(dc->tmp_regs)); > dc->tmp_regs[n].idx = rdst; > return dc->tmp_regs[n].val = tcg_temp_new_i64(); > } > > In this way you can in fact call dest_gr twice within load_add and everything > will Just Work. > For me, the code is fine (and reset dc->n_tmp_regs for each bundle). Thanks. -- Chen Gang Open, share, and attitude like air, water, and life which God blessed