| Message ID | 1597765847-16637-29-git-send-email-tsimpson@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Hexagon patch series | expand |
On 8/18/20 8:50 AM, Taylor Simpson wrote: > Helpers for reading and writing registers > Helpers for load-locked/store-conditional > > Signed-off-by: Taylor Simpson <tsimpson@quicinc.com> > --- > target/hexagon/genptr_helpers.h | 244 ++++++++++++++++++++++++++++++++++++++++ > target/hexagon/op_helper.c | 18 +++ > 2 files changed, 262 insertions(+) > create mode 100644 target/hexagon/genptr_helpers.h > > diff --git a/target/hexagon/genptr_helpers.h b/target/hexagon/genptr_helpers.h > new file mode 100644 > index 0000000..ffcb1e3 > --- /dev/null > +++ b/target/hexagon/genptr_helpers.h > @@ -0,0 +1,244 @@ > +/* > + * Copyright(c) 2019-2020 Qualcomm Innovation Center, Inc. All Rights Reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, see <http://www.gnu.org/licenses/>. > + */ > + > +#ifndef HEXAGON_GENPTR_HELPERS_H > +#define HEXAGON_GENPTR_HELPERS_H > + > +#include "tcg/tcg.h" > + > +static inline TCGv gen_read_reg(TCGv result, int num) > +{ > + tcg_gen_mov_tl(result, hex_gpr[num]); > + return result; > +} > + > +static inline TCGv gen_read_preg(TCGv pred, uint8_t num) > +{ > + tcg_gen_mov_tl(pred, hex_pred[num]); > + return pred; > +} > + > +static inline void gen_log_reg_write(int rnum, TCGv val, int slot, > + int is_predicated) These are quite large. Why are they marked inline? > + /* Low word */ > + tcg_gen_extrl_i64_i32(val32, val); > + tcg_gen_mov_tl(hex_new_value[rnum], val32); Why are you extracting into a temporary? This could be done with tcg_gen_extr_i64_i32(hex_new_value[rnum], hex_new_value[rnum + 1], val); > +static inline void gen_read_p3_0(TCGv control_reg) > +{ > + TCGv pval = tcg_temp_new(); > + int i; > + tcg_gen_movi_tl(control_reg, 0); > + for (i = NUM_PREGS - 1; i >= 0; i--) { > + tcg_gen_shli_tl(control_reg, control_reg, 8); > + tcg_gen_andi_tl(pval, hex_pred[i], 0xff); > + tcg_gen_or_tl(control_reg, control_reg, pval); tcg_gen_deposit_tl(control_reg, control_reg, hex_pred[i], i * 8, 8); > + for (i = 0; i < NUM_PREGS; i++) { > + tcg_gen_andi_tl(pred_val, control_reg, 0xff); > + tcg_gen_mov_tl(hex_pred[i], pred_val); > + tcg_gen_shri_tl(control_reg, control_reg, 8); tcg_gen_extract_tl(hex_pred[i], control_reg, i * 8, 8); > +static inline void log_store32(CPUHexagonState *env, target_ulong addr, > + int32_t val, int width, int slot) > +{ > + HEX_DEBUG_LOG("log_store%d(0x%x, %d [0x%x])\n", width, addr, val, val); > + env->mem_log_stores[slot].va = addr; > + env->mem_log_stores[slot].width = width; > + env->mem_log_stores[slot].data32 = val; > +} > + > +static inline void log_store64(CPUHexagonState *env, target_ulong addr, > + int64_t val, int width, int slot) > +{ > + HEX_DEBUG_LOG("log_store%d(0x%x, %ld [0x%lx])\n", width, addr, val, val); > + env->mem_log_stores[slot].va = addr; > + env->mem_log_stores[slot].width = width; > + env->mem_log_stores[slot].data64 = val; > +} ... or fold this re-addition back into where it was accidentally removed. ;-) r~
> -----Original Message----- > From: Richard Henderson <richard.henderson@linaro.org> > Sent: Friday, August 28, 2020 7:49 PM > To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org > Cc: philmd@redhat.com; laurent@vivier.eu; riku.voipio@iki.fi; > aleksandar.m.mail@gmail.com; ale@rev.ng > Subject: Re: [RFC PATCH v3 28/34] Hexagon (target/hexagon) TCG > generation helpers > > On 8/18/20 8:50 AM, Taylor Simpson wrote: > > Helpers for reading and writing registers > > Helpers for load-locked/store-conditional > > > > Signed-off-by: Taylor Simpson <tsimpson@quicinc.com> > > --- > > target/hexagon/genptr_helpers.h | 244 > ++++++++++++++++++++++++++++++++++++++++ > > target/hexagon/op_helper.c | 18 +++ > > 2 files changed, 262 insertions(+) > > create mode 100644 target/hexagon/genptr_helpers.h > > > > diff --git a/target/hexagon/genptr_helpers.h > b/target/hexagon/genptr_helpers.h > > new file mode 100644 > > index 0000000..ffcb1e3 > > --- /dev/null > > +++ b/target/hexagon/genptr_helpers.h > > @@ -0,0 +1,244 @@ > > + > > +static inline void gen_log_reg_write(int rnum, TCGv val, int slot, > > + int is_predicated) > > These are quite large. Why are they marked inline? Since this is a header file, it prevents the compiler from complaining when they aren't used. > > > + /* Low word */ > > + tcg_gen_extrl_i64_i32(val32, val); > > + tcg_gen_mov_tl(hex_new_value[rnum], val32); > > Why are you extracting into a temporary? > This could be done with > > tcg_gen_extr_i64_i32(hex_new_value[rnum], > hex_new_value[rnum + 1], val); OK > > +static inline void gen_read_p3_0(TCGv control_reg) > > +{ > > + TCGv pval = tcg_temp_new(); > > + int i; > > + tcg_gen_movi_tl(control_reg, 0); > > + for (i = NUM_PREGS - 1; i >= 0; i--) { > > + tcg_gen_shli_tl(control_reg, control_reg, 8); > > + tcg_gen_andi_tl(pval, hex_pred[i], 0xff); > > + tcg_gen_or_tl(control_reg, control_reg, pval); > > tcg_gen_deposit_tl(control_reg, control_reg, > hex_pred[i], i * 8, 8); OK > > + for (i = 0; i < NUM_PREGS; i++) { > > + tcg_gen_andi_tl(pred_val, control_reg, 0xff); > > + tcg_gen_mov_tl(hex_pred[i], pred_val); > > + tcg_gen_shri_tl(control_reg, control_reg, 8); > > tcg_gen_extract_tl(hex_pred[i], control_reg, i * 8, 8); OK > > +static inline void log_store32(CPUHexagonState *env, target_ulong addr, > > + int32_t val, int width, int slot) > > +{ > > + HEX_DEBUG_LOG("log_store%d(0x%x, %d [0x%x])\n", width, addr, val, > val); > > + env->mem_log_stores[slot].va = addr; > > + env->mem_log_stores[slot].width = width; > > + env->mem_log_stores[slot].data32 = val; > > +} > > + > > +static inline void log_store64(CPUHexagonState *env, target_ulong addr, > > + int64_t val, int width, int slot) > > +{ > > + HEX_DEBUG_LOG("log_store%d(0x%x, %ld [0x%lx])\n", width, addr, > val, val); > > + env->mem_log_stores[slot].va = addr; > > + env->mem_log_stores[slot].width = width; > > + env->mem_log_stores[slot].data64 = val; > > +} > > ... or fold this re-addition back into where it was accidentally removed. ;-) Could you elaborate?
On 8/30/20 12:53 PM, Taylor Simpson wrote: >>> +++ b/target/hexagon/genptr_helpers.h >>> @@ -0,0 +1,244 @@ >>> + >>> +static inline void gen_log_reg_write(int rnum, TCGv val, int slot, >>> + int is_predicated) >> >> These are quite large. Why are they marked inline? > > Since this is a header file, it prevents the compiler from complaining when they aren't used. Ok, why are they in a header file? Why would they be unused, come to that? The header file is used exactly once, by genptr.c. Seems to me they could just as well be *in* genptr.c. If the functions are not used, just remove them? >>> +static inline void log_store32(CPUHexagonState *env, target_ulong addr, >>> + int32_t val, int width, int slot) >>> +{ >>> + HEX_DEBUG_LOG("log_store%d(0x%x, %d [0x%x])\n", width, addr, val, >> val); >>> + env->mem_log_stores[slot].va = addr; >>> + env->mem_log_stores[slot].width = width; >>> + env->mem_log_stores[slot].data32 = val; >>> +} >>> + >>> +static inline void log_store64(CPUHexagonState *env, target_ulong addr, >>> + int64_t val, int width, int slot) >>> +{ >>> + HEX_DEBUG_LOG("log_store%d(0x%x, %ld [0x%lx])\n", width, addr, >> val, val); >>> + env->mem_log_stores[slot].va = addr; >>> + env->mem_log_stores[slot].width = width; >>> + env->mem_log_stores[slot].data64 = val; >>> +} >> >> ... or fold this re-addition back into where it was accidentally removed. ;-) > > Could you elaborate? You added this code in one patch (didn't check which), removed it in patch 26, and re-added it here in patch 28. r~
> -----Original Message----- > From: Richard Henderson <richard.henderson@linaro.org> > Sent: Sunday, August 30, 2020 2:52 PM > To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org > Cc: philmd@redhat.com; laurent@vivier.eu; riku.voipio@iki.fi; > aleksandar.m.mail@gmail.com; ale@rev.ng > Subject: Re: [RFC PATCH v3 28/34] Hexagon (target/hexagon) TCG > generation helpers > > On 8/30/20 12:53 PM, Taylor Simpson wrote: > >>> +++ b/target/hexagon/genptr_helpers.h > >>> @@ -0,0 +1,244 @@ > >>> + > >>> +static inline void gen_log_reg_write(int rnum, TCGv val, int slot, > >>> + int is_predicated) > >> > >> These are quite large. Why are they marked inline? > > > > Since this is a header file, it prevents the compiler from complaining when > they aren't used. > > Ok, why are they in a header file? > Why would they be unused, come to that? > > The header file is used exactly once, by genptr.c. Seems to me they could > just > as well be *in* genptr.c. > > If the functions are not used, just remove them? I could have sworn it was included in more than one C file. I'll move the contents to genptr.c. > >>> +static inline void log_store32(CPUHexagonState *env, target_ulong > addr, > >>> + int32_t val, int width, int slot) > >>> +{ > >>> + HEX_DEBUG_LOG("log_store%d(0x%x, %d [0x%x])\n", width, addr, > val, > >> val); > >>> + env->mem_log_stores[slot].va = addr; > >>> + env->mem_log_stores[slot].width = width; > >>> + env->mem_log_stores[slot].data32 = val; > >>> +} > >>> + > >>> +static inline void log_store64(CPUHexagonState *env, target_ulong > addr, > >>> + int64_t val, int width, int slot) > >>> +{ > >>> + HEX_DEBUG_LOG("log_store%d(0x%x, %ld [0x%lx])\n", width, addr, > >> val, val); > >>> + env->mem_log_stores[slot].va = addr; > >>> + env->mem_log_stores[slot].width = width; > >>> + env->mem_log_stores[slot].data64 = val; > >>> +} > >> > >> ... or fold this re-addition back into where it was accidentally removed. ;-) > > > > Could you elaborate? > > You added this code in one patch (didn't check which), removed it in patch > 26, > and re-added it here in patch 28. My apologies, this is my screwing up the git rebase. I'll fix it. > > > r~
diff --git a/target/hexagon/genptr_helpers.h b/target/hexagon/genptr_helpers.h new file mode 100644 index 0000000..ffcb1e3 --- /dev/null +++ b/target/hexagon/genptr_helpers.h @@ -0,0 +1,244 @@ +/* + * Copyright(c) 2019-2020 Qualcomm Innovation Center, Inc. All Rights Reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, see <http://www.gnu.org/licenses/>. + */ + +#ifndef HEXAGON_GENPTR_HELPERS_H +#define HEXAGON_GENPTR_HELPERS_H + +#include "tcg/tcg.h" + +static inline TCGv gen_read_reg(TCGv result, int num) +{ + tcg_gen_mov_tl(result, hex_gpr[num]); + return result; +} + +static inline TCGv gen_read_preg(TCGv pred, uint8_t num) +{ + tcg_gen_mov_tl(pred, hex_pred[num]); + return pred; +} + +static inline void gen_log_reg_write(int rnum, TCGv val, int slot, + int is_predicated) +{ + if (is_predicated) { + TCGv one = tcg_const_tl(1); + TCGv zero = tcg_const_tl(0); + TCGv slot_mask = tcg_temp_new(); + + tcg_gen_andi_tl(slot_mask, hex_slot_cancelled, 1 << slot); + tcg_gen_movcond_tl(TCG_COND_EQ, hex_new_value[rnum], slot_mask, zero, + val, hex_new_value[rnum]); +#if HEX_DEBUG + /* Do this so HELPER(debug_commit_end) will know */ + tcg_gen_movcond_tl(TCG_COND_EQ, hex_reg_written[rnum], slot_mask, zero, + one, hex_reg_written[rnum]); +#endif + + tcg_temp_free(one); + tcg_temp_free(zero); + tcg_temp_free(slot_mask); + } else { + tcg_gen_mov_tl(hex_new_value[rnum], val); +#if HEX_DEBUG + /* Do this so HELPER(debug_commit_end) will know */ + tcg_gen_movi_tl(hex_reg_written[rnum], 1); +#endif + } +} + +static inline void gen_log_reg_write_pair(int rnum, TCGv_i64 val, int slot, + int is_predicated) +{ + TCGv val32 = tcg_temp_new(); + + if (is_predicated) { + TCGv one = tcg_const_tl(1); + TCGv zero = tcg_const_tl(0); + TCGv slot_mask = tcg_temp_new(); + + tcg_gen_andi_tl(slot_mask, hex_slot_cancelled, 1 << slot); + /* Low word */ + tcg_gen_extrl_i64_i32(val32, val); + tcg_gen_movcond_tl(TCG_COND_EQ, hex_new_value[rnum], slot_mask, zero, + val32, hex_new_value[rnum]); +#if HEX_DEBUG + /* Do this so HELPER(debug_commit_end) will know */ + tcg_gen_movcond_tl(TCG_COND_EQ, hex_reg_written[rnum], + slot_mask, zero, + one, hex_reg_written[rnum]); +#endif + + /* High word */ + tcg_gen_extrh_i64_i32(val32, val); + tcg_gen_movcond_tl(TCG_COND_EQ, hex_new_value[rnum + 1], + slot_mask, zero, + val32, hex_new_value[rnum + 1]); +#if HEX_DEBUG + /* Do this so HELPER(debug_commit_end) will know */ + tcg_gen_movcond_tl(TCG_COND_EQ, hex_reg_written[rnum + 1], + slot_mask, zero, + one, hex_reg_written[rnum + 1]); +#endif + + tcg_temp_free(one); + tcg_temp_free(zero); + tcg_temp_free(slot_mask); + } else { + /* Low word */ + tcg_gen_extrl_i64_i32(val32, val); + tcg_gen_mov_tl(hex_new_value[rnum], val32); +#if HEX_DEBUG + /* Do this so HELPER(debug_commit_end) will know */ + tcg_gen_movi_tl(hex_reg_written[rnum], 1); +#endif + + /* High word */ + tcg_gen_extrh_i64_i32(val32, val); + tcg_gen_mov_tl(hex_new_value[rnum + 1], val32); +#if HEX_DEBUG + /* Do this so HELPER(debug_commit_end) will know */ + tcg_gen_movi_tl(hex_reg_written[rnum + 1], 1); +#endif + } + + tcg_temp_free(val32); +} + +static inline void gen_log_pred_write(int pnum, TCGv val) +{ + TCGv zero = tcg_const_tl(0); + TCGv base_val = tcg_temp_new(); + TCGv and_val = tcg_temp_new(); + TCGv pred_written = tcg_temp_new(); + + /* Multiple writes to the same preg are and'ed together */ + tcg_gen_andi_tl(base_val, val, 0xff); + tcg_gen_and_tl(and_val, base_val, hex_new_pred_value[pnum]); + tcg_gen_andi_tl(pred_written, hex_pred_written, 1 << pnum); + tcg_gen_movcond_tl(TCG_COND_NE, hex_new_pred_value[pnum], + pred_written, zero, + and_val, base_val); + tcg_gen_ori_tl(hex_pred_written, hex_pred_written, 1 << pnum); + + tcg_temp_free(zero); + tcg_temp_free(base_val); + tcg_temp_free(and_val); + tcg_temp_free(pred_written); +} + +static inline void gen_read_p3_0(TCGv control_reg) +{ + TCGv pval = tcg_temp_new(); + int i; + tcg_gen_movi_tl(control_reg, 0); + for (i = NUM_PREGS - 1; i >= 0; i--) { + tcg_gen_shli_tl(control_reg, control_reg, 8); + tcg_gen_andi_tl(pval, hex_pred[i], 0xff); + tcg_gen_or_tl(control_reg, control_reg, pval); + } + tcg_temp_free(pval); +} + +static inline void gen_write_p3_0(TCGv tmp) +{ + TCGv control_reg = tcg_temp_new(); + TCGv pred_val = tcg_temp_new(); + int i; + + tcg_gen_mov_tl(control_reg, tmp); + for (i = 0; i < NUM_PREGS; i++) { + tcg_gen_andi_tl(pred_val, control_reg, 0xff); + tcg_gen_mov_tl(hex_pred[i], pred_val); + tcg_gen_shri_tl(control_reg, control_reg, 8); + } + tcg_temp_free(control_reg); + tcg_temp_free(pred_val); +} + +static inline void gen_load_locked4u(TCGv dest, TCGv vaddr, int mem_index) +{ + tcg_gen_qemu_ld32u(dest, vaddr, mem_index); + tcg_gen_mov_tl(hex_llsc_addr, vaddr); + tcg_gen_mov_tl(hex_llsc_val, dest); +} + +static inline void gen_load_locked8u(TCGv_i64 dest, TCGv vaddr, int mem_index) +{ + tcg_gen_qemu_ld64(dest, vaddr, mem_index); + tcg_gen_mov_tl(hex_llsc_addr, vaddr); + tcg_gen_mov_i64(hex_llsc_val_i64, dest); +} + +static inline void gen_store_conditional4(CPUHexagonState *env, + DisasContext *ctx, int prednum, + TCGv pred, TCGv vaddr, TCGv src) +{ + TCGLabel *fail = gen_new_label(); + TCGLabel *done = gen_new_label(); + + tcg_gen_brcond_tl(TCG_COND_NE, vaddr, hex_llsc_addr, fail); + + TCGv one = tcg_const_tl(0xff); + TCGv zero = tcg_const_tl(0); + TCGv tmp = tcg_temp_new(); + tcg_gen_atomic_cmpxchg_tl(tmp, hex_llsc_addr, hex_llsc_val, src, + ctx->mem_idx, MO_32); + tcg_gen_movcond_tl(TCG_COND_EQ, hex_pred[prednum], tmp, hex_llsc_val, + one, zero); + tcg_temp_free(one); + tcg_temp_free(zero); + tcg_temp_free(tmp); + tcg_gen_br(done); + + gen_set_label(fail); + tcg_gen_movi_tl(pred, 0); + + gen_set_label(done); + tcg_gen_movi_tl(hex_llsc_addr, ~0); +} + +static inline void gen_store_conditional8(CPUHexagonState *env, + DisasContext *ctx, int prednum, + TCGv pred, TCGv vaddr, TCGv_i64 src) +{ + TCGLabel *fail = gen_new_label(); + TCGLabel *done = gen_new_label(); + + tcg_gen_brcond_tl(TCG_COND_NE, vaddr, hex_llsc_addr, fail); + + TCGv_i64 one = tcg_const_i64(0xff); + TCGv_i64 zero = tcg_const_i64(0); + TCGv_i64 tmp = tcg_temp_new_i64(); + tcg_gen_atomic_cmpxchg_i64(tmp, hex_llsc_addr, hex_llsc_val_i64, src, + ctx->mem_idx, MO_64); + tcg_gen_movcond_i64(TCG_COND_EQ, tmp, tmp, hex_llsc_val_i64, + one, zero); + tcg_gen_extrl_i64_i32(hex_pred[prednum], tmp); + tcg_temp_free_i64(one); + tcg_temp_free_i64(zero); + tcg_temp_free_i64(tmp); + tcg_gen_br(done); + + gen_set_label(fail); + tcg_gen_movi_tl(pred, 0); + + gen_set_label(done); + tcg_gen_movi_tl(hex_llsc_addr, ~0); +} + +#endif diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c index 2ea4e70..a234cf6 100644 --- a/target/hexagon/op_helper.c +++ b/target/hexagon/op_helper.c @@ -88,6 +88,24 @@ static inline void log_pred_write(CPUHexagonState *env, int pnum, } } +static inline void log_store32(CPUHexagonState *env, target_ulong addr, + int32_t val, int width, int slot) +{ + HEX_DEBUG_LOG("log_store%d(0x%x, %d [0x%x])\n", width, addr, val, val); + env->mem_log_stores[slot].va = addr; + env->mem_log_stores[slot].width = width; + env->mem_log_stores[slot].data32 = val; +} + +static inline void log_store64(CPUHexagonState *env, target_ulong addr, + int64_t val, int width, int slot) +{ + HEX_DEBUG_LOG("log_store%d(0x%x, %ld [0x%lx])\n", width, addr, val, val); + env->mem_log_stores[slot].va = addr; + env->mem_log_stores[slot].width = width; + env->mem_log_stores[slot].data64 = val; +} + static inline void write_new_pc(CPUHexagonState *env, target_ulong addr) { HEX_DEBUG_LOG("write_new_pc(0x" TARGET_FMT_lx ")\n", addr);
Helpers for reading and writing registers Helpers for load-locked/store-conditional Signed-off-by: Taylor Simpson <tsimpson@quicinc.com> --- target/hexagon/genptr_helpers.h | 244 ++++++++++++++++++++++++++++++++++++++++ target/hexagon/op_helper.c | 18 +++ 2 files changed, 262 insertions(+) create mode 100644 target/hexagon/genptr_helpers.h