Message ID | 1458815961-31979-9-git-send-email-sergey.fedorov@linaro.org |
---|---|
State | New |
Headers | show |
sergey.fedorov@linaro.org writes: > From: Sergey Fedorov <serge.fdrv@gmail.com> > > Unify the code of this function with tb_jmp_remove_from_list(). Making > these functions similar improves their readability. Also this could be a > step towards making this function thread-safe. > > Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com> > Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org> > --- > translate-all.c | 20 ++++++++------------ > 1 file changed, 8 insertions(+), 12 deletions(-) > > diff --git a/translate-all.c b/translate-all.c > index 8b4bfa713bf7..56c77a72773d 100644 > --- a/translate-all.c > +++ b/translate-all.c > @@ -962,25 +962,21 @@ static inline void tb_reset_jump(TranslationBlock *tb, int n) > /* remove any jumps to the TB */ > static inline void tb_jmp_unlink(TranslationBlock *tb) > { > - uintptr_t tb1, tb2; > + TranslationBlock *tb1; > + uintptr_t *ptb; > unsigned int n1; > > - tb1 = tb->jmp_list_first; > + ptb = &tb->jmp_list_first; > for (;;) { > - TranslationBlock *tmp_tb; > - n1 = tb1 & 3; > + n1 = *ptb & 3; > + tb1 = (TranslationBlock *)(*ptb & ~3); I would hope the compiler saw through the duplicate indirect accesses but maybe: uintptr_t *ptb, ntb; and ntb = *ptb; n1 = ntb & 3; tb1 = (TranslationBlock *)(ntb & ~3); would be clearer? > if (n1 == 2) { > break; > } > - tmp_tb = (TranslationBlock *)(tb1 & ~3); > - tb2 = tmp_tb->jmp_list_next[n1]; > - tb_reset_jump(tmp_tb, n1); > - tmp_tb->jmp_list_next[n1] = (uintptr_t)NULL; > - tb1 = tb2; > + tb_reset_jump(tb1, n1); > + *ptb = tb1->jmp_list_next[n1]; > + tb1->jmp_list_next[n1] = (uintptr_t)NULL; > } > - > - assert(((uintptr_t)tb & 3) == 0); > - tb->jmp_list_first = (uintptr_t)tb | 2; /* fail safe */ > } > > /* invalidate one TB */ Otherwise: Reviewed-by: Alex Bennée <alex.bennee@linaro.org> -- Alex Bennée
On 24/03/16 18:36, Alex Bennée wrote: >> diff --git a/translate-all.c b/translate-all.c >> > index 8b4bfa713bf7..56c77a72773d 100644 >> > --- a/translate-all.c >> > +++ b/translate-all.c >> > @@ -962,25 +962,21 @@ static inline void tb_reset_jump(TranslationBlock *tb, int n) >> > /* remove any jumps to the TB */ >> > static inline void tb_jmp_unlink(TranslationBlock *tb) >> > { >> > - uintptr_t tb1, tb2; >> > + TranslationBlock *tb1; >> > + uintptr_t *ptb; >> > unsigned int n1; >> > >> > - tb1 = tb->jmp_list_first; >> > + ptb = &tb->jmp_list_first; >> > for (;;) { >> > - TranslationBlock *tmp_tb; >> > - n1 = tb1 & 3; >> > + n1 = *ptb & 3; >> > + tb1 = (TranslationBlock *)(*ptb & ~3); > I would hope the compiler saw through the duplicate indirect accesses > but maybe: > > uintptr_t *ptb, ntb; > > and > > ntb = *ptb; > n1 = ntb & 3; > tb1 = (TranslationBlock *)(ntb & ~3); > > would be clearer? > Why not? :) Will do this. Thanks, Sergey
On 24/03/16 18:42, Sergey Fedorov wrote: > On 24/03/16 18:36, Alex Bennée wrote: >>> diff --git a/translate-all.c b/translate-all.c >>>> index 8b4bfa713bf7..56c77a72773d 100644 >>>> --- a/translate-all.c >>>> +++ b/translate-all.c >>>> @@ -962,25 +962,21 @@ static inline void tb_reset_jump(TranslationBlock *tb, int n) >>>> /* remove any jumps to the TB */ >>>> static inline void tb_jmp_unlink(TranslationBlock *tb) >>>> { >>>> - uintptr_t tb1, tb2; >>>> + TranslationBlock *tb1; >>>> + uintptr_t *ptb; >>>> unsigned int n1; >>>> >>>> - tb1 = tb->jmp_list_first; >>>> + ptb = &tb->jmp_list_first; >>>> for (;;) { >>>> - TranslationBlock *tmp_tb; >>>> - n1 = tb1 & 3; >>>> + n1 = *ptb & 3; >>>> + tb1 = (TranslationBlock *)(*ptb & ~3); >> I would hope the compiler saw through the duplicate indirect accesses >> but maybe: >> >> uintptr_t *ptb, ntb; >> >> and >> >> ntb = *ptb; >> n1 = ntb & 3; >> tb1 = (TranslationBlock *)(ntb & ~3); >> >> would be clearer? >> > Why not? :) Will do this. I'll do the same in tb_remove_from_jmp_list(). It will go to the patch 8 "tcg: Use uintptr_t type for jmp_list_{next|first} fields of TB". -Sergey
diff --git a/translate-all.c b/translate-all.c index 8b4bfa713bf7..56c77a72773d 100644 --- a/translate-all.c +++ b/translate-all.c @@ -962,25 +962,21 @@ static inline void tb_reset_jump(TranslationBlock *tb, int n) /* remove any jumps to the TB */ static inline void tb_jmp_unlink(TranslationBlock *tb) { - uintptr_t tb1, tb2; + TranslationBlock *tb1; + uintptr_t *ptb; unsigned int n1; - tb1 = tb->jmp_list_first; + ptb = &tb->jmp_list_first; for (;;) { - TranslationBlock *tmp_tb; - n1 = tb1 & 3; + n1 = *ptb & 3; + tb1 = (TranslationBlock *)(*ptb & ~3); if (n1 == 2) { break; } - tmp_tb = (TranslationBlock *)(tb1 & ~3); - tb2 = tmp_tb->jmp_list_next[n1]; - tb_reset_jump(tmp_tb, n1); - tmp_tb->jmp_list_next[n1] = (uintptr_t)NULL; - tb1 = tb2; + tb_reset_jump(tb1, n1); + *ptb = tb1->jmp_list_next[n1]; + tb1->jmp_list_next[n1] = (uintptr_t)NULL; } - - assert(((uintptr_t)tb & 3) == 0); - tb->jmp_list_first = (uintptr_t)tb | 2; /* fail safe */ } /* invalidate one TB */