| Message ID | BANLkTinNQ8Erh_Xvhg=Rm1W74SjexO3uyA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Sun, 26 Jun 2011, Blue Swirl wrote: > Use TCG_REG_CALL_STACK instead of TCG_REG_R1 etc. for consistency. > This i'd rather avoid. [..snip..]
On Tue, Jun 28, 2011 at 1:51 AM, malc <av1474@comtv.ru> wrote: > On Sun, 26 Jun 2011, Blue Swirl wrote: > >> Use TCG_REG_CALL_STACK instead of TCG_REG_R1 etc. for consistency. >> > > This i'd rather avoid. Why? In addition to the consistency among targets, a magic constant is replaced with a symbol which improves the documenting abilities and readability of the code.
On Sat, 2 Jul 2011, Blue Swirl wrote: > On Tue, Jun 28, 2011 at 1:51 AM, malc <av1474@comtv.ru> wrote: > > On Sun, 26 Jun 2011, Blue Swirl wrote: > > > >> Use TCG_REG_CALL_STACK instead of TCG_REG_R1 etc. for consistency. > >> > > > > This i'd rather avoid. > > Why? In addition to the consistency among targets, a magic constant is > replaced with a symbol which improves the documenting abilities and > readability of the code. Makes it harder to read for me personally.
On Sat, Jul 2, 2011 at 12:06 AM, malc <av1474@comtv.ru> wrote: > On Sat, 2 Jul 2011, Blue Swirl wrote: > >> On Tue, Jun 28, 2011 at 1:51 AM, malc <av1474@comtv.ru> wrote: >> > On Sun, 26 Jun 2011, Blue Swirl wrote: >> > >> >> Use TCG_REG_CALL_STACK instead of TCG_REG_R1 etc. for consistency. >> >> >> > >> > This i'd rather avoid. >> >> Why? In addition to the consistency among targets, a magic constant is >> replaced with a symbol which improves the documenting abilities and >> readability of the code. > > Makes it harder to read for me personally. What about other people reading the code? With TCG_REG_CALL_STACK it's pretty clear what is the purpose of the register, with '1' it is assumed that the reader happens to know it. I don't think the exact value is so interesting, or is register 1 somehow different from others?
Am 02.07.2011 um 11:04 schrieb Blue Swirl: > On Sat, Jul 2, 2011 at 12:06 AM, malc <av1474@comtv.ru> wrote: >> On Sat, 2 Jul 2011, Blue Swirl wrote: >> >>> On Tue, Jun 28, 2011 at 1:51 AM, malc <av1474@comtv.ru> wrote: >>>> On Sun, 26 Jun 2011, Blue Swirl wrote: >>>> >>>>> Use TCG_REG_CALL_STACK instead of TCG_REG_R1 etc. for consistency. >>>>> >>>> >>>> This i'd rather avoid. >>> >>> Why? In addition to the consistency among targets, a magic >>> constant is >>> replaced with a symbol which improves the documenting abilities and >>> readability of the code. >> >> Makes it harder to read for me personally. > > What about other people reading the code? With TCG_REG_CALL_STACK it's > pretty clear what is the purpose of the register, with '1' it is > assumed that the reader happens to know it. I don't think the exact > value is so interesting, or is register 1 somehow different from > others? I second malc's notion. This is ppc-specific code, and GPR1 (r1) is the designated stack pointer, that has nothing to do with TCG. Using some generic TCG define hides from the reader which reg is being used, potentially leading to nonsensical moves from r1 to TCG_... or vice versa. If we do want a define, I would suggest something ppc-specific like r1 or sp. Andreas
On Sat, Jul 2, 2011 at 1:02 PM, Andreas Färber <andreas.faerber@web.de> wrote: > Am 02.07.2011 um 11:04 schrieb Blue Swirl: > >> On Sat, Jul 2, 2011 at 12:06 AM, malc <av1474@comtv.ru> wrote: >>> >>> On Sat, 2 Jul 2011, Blue Swirl wrote: >>> >>>> On Tue, Jun 28, 2011 at 1:51 AM, malc <av1474@comtv.ru> wrote: >>>>> >>>>> On Sun, 26 Jun 2011, Blue Swirl wrote: >>>>> >>>>>> Use TCG_REG_CALL_STACK instead of TCG_REG_R1 etc. for consistency. >>>>>> >>>>> >>>>> This i'd rather avoid. >>>> >>>> Why? In addition to the consistency among targets, a magic constant is >>>> replaced with a symbol which improves the documenting abilities and >>>> readability of the code. >>> >>> Makes it harder to read for me personally. >> >> What about other people reading the code? With TCG_REG_CALL_STACK it's >> pretty clear what is the purpose of the register, with '1' it is >> assumed that the reader happens to know it. I don't think the exact >> value is so interesting, or is register 1 somehow different from >> others? > > I second malc's notion. This is ppc-specific code, and GPR1 (r1) is the > designated stack pointer, that has nothing to do with TCG. Using some > generic TCG define hides from the reader which reg is being used, > potentially leading to nonsensical moves from r1 to TCG_... or vice versa. OK, I'll drop the patch. > If we do want a define, I would suggest something ppc-specific like r1 or > sp. > > Andreas >
diff --git a/tcg/ppc/tcg-target.c b/tcg/ppc/tcg-target.c index dde4e18..5ab5d6d 100644 --- a/tcg/ppc/tcg-target.c +++ b/tcg/ppc/tcg-target.c @@ -921,15 +921,17 @@ static void tcg_target_qemu_prologue (TCGContext *s) } #endif tcg_out32 (s, MFSPR | RT (0) | LR); - tcg_out32 (s, STWU | RS (1) | RA (1) | (-frame_size & 0xffff)); + tcg_out32 (s, STWU | RS (TCG_REG_CALL_STACK) | RA (TCG_REG_CALL_STACK) + | (-frame_size & 0xffff)); for (i = 0; i < ARRAY_SIZE (tcg_target_callee_save_regs); ++i) tcg_out32 (s, (STW | RS (tcg_target_callee_save_regs[i]) - | RA (1) + | RA (TCG_REG_CALL_STACK) | (i * 4 + LINKAGE_AREA_SIZE + TCG_STATIC_CALL_ARGS_SIZE) ) ); - tcg_out32 (s, STW | RS (0) | RA (1) | (frame_size + LR_OFFSET)); + tcg_out32 (s, STW | RS (0) | RA (TCG_REG_CALL_STACK) + | (frame_size + LR_OFFSET)); #ifdef CONFIG_USE_GUEST_BASE if (GUEST_BASE) { @@ -946,13 +948,15 @@ static void tcg_target_qemu_prologue (TCGContext *s) for (i = 0; i < ARRAY_SIZE (tcg_target_callee_save_regs); ++i) tcg_out32 (s, (LWZ | RT (tcg_target_callee_save_regs[i]) - | RA (1) + | RA (TCG_REG_CALL_STACK) | (i * 4 + LINKAGE_AREA_SIZE + TCG_STATIC_CALL_ARGS_SIZE) ) ); - tcg_out32 (s, LWZ | RT (0) | RA (1) | (frame_size + LR_OFFSET)); + tcg_out32 (s, LWZ | RT (0) | RA (TCG_REG_CALL_STACK) + | (frame_size + LR_OFFSET)); tcg_out32 (s, MTSPR | RS (0) | LR); - tcg_out32 (s, ADDI | RT (1) | RA (1) | frame_size); + tcg_out32 (s, ADDI | RT (TCG_REG_CALL_STACK) | RA (TCG_REG_CALL_STACK) + | frame_size); tcg_out32 (s, BCLR | BO_ALWAYS); } @@ -1910,7 +1914,7 @@ static void tcg_target_init(TCGContext *s) tcg_regset_clear(s->reserved_regs); tcg_regset_set_reg(s->reserved_regs, TCG_REG_R0); - tcg_regset_set_reg(s->reserved_regs, TCG_REG_R1); + tcg_regset_set_reg(s->reserved_regs, TCG_REG_CALL_STACK); #ifndef _CALL_DARWIN tcg_regset_set_reg(s->reserved_regs, TCG_REG_R2); #endif diff --git a/tcg/ppc64/tcg-target.c b/tcg/ppc64/tcg-target.c index 3218438..5f79342 100644 --- a/tcg/ppc64/tcg-target.c +++ b/tcg/ppc64/tcg-target.c @@ -887,15 +887,16 @@ static void tcg_target_qemu_prologue (TCGContext *s) /* Prologue */ tcg_out32 (s, MFSPR | RT (0) | LR); - tcg_out32 (s, STDU | RS (1) | RA (1) | (-frame_size & 0xffff)); + tcg_out32 (s, STDU | RS (TCG_REG_CALL_STACK) | RA (TCG_REG_CALL_STACK) + | (-frame_size & 0xffff)); for (i = 0; i < ARRAY_SIZE (tcg_target_callee_save_regs); ++i) tcg_out32 (s, (STD | RS (tcg_target_callee_save_regs[i]) - | RA (1) + | RA (TCG_REG_CALL_STACK) | (i * 8 + 48 + TCG_STATIC_CALL_ARGS_SIZE) ) ); - tcg_out32 (s, STD | RS (0) | RA (1) | (frame_size + 16)); + tcg_out32 (s, STD | RS (0) | RA (TCG_REG_CALL_STACK) | (frame_size + 16)); #ifdef CONFIG_USE_GUEST_BASE if (GUEST_BASE) { @@ -914,13 +915,14 @@ static void tcg_target_qemu_prologue (TCGContext *s) for (i = 0; i < ARRAY_SIZE (tcg_target_callee_save_regs); ++i) tcg_out32 (s, (LD | RT (tcg_target_callee_save_regs[i]) - | RA (1) + | RA (TCG_REG_CALL_STACK) | (i * 8 + 48 + TCG_STATIC_CALL_ARGS_SIZE) ) ); - tcg_out32 (s, LD | RT (0) | RA (1) | (frame_size + 16)); + tcg_out32 (s, LD | RT (0) | RA (TCG_REG_CALL_STACK) | (frame_size + 16)); tcg_out32 (s, MTSPR | RS (0) | LR); - tcg_out32 (s, ADDI | RT (1) | RA (1) | frame_size); + tcg_out32 (s, ADDI | RT (TCG_REG_CALL_STACK) | RA (TCG_REG_CALL_STACK) + | frame_size); tcg_out32 (s, BCLR | BO_ALWAYS); } @@ -1689,7 +1691,7 @@ static void tcg_target_init (TCGContext *s) tcg_regset_clear (s->reserved_regs); tcg_regset_set_reg (s->reserved_regs, TCG_REG_R0); - tcg_regset_set_reg (s->reserved_regs, TCG_REG_R1); + tcg_regset_set_reg (s->reserved_regs, TCG_REG_CALL_STACK); #ifndef __APPLE__ tcg_regset_set_reg (s->reserved_regs, TCG_REG_R2); #endif
Use TCG_REG_CALL_STACK instead of TCG_REG_R1 etc. for consistency. Signed-off-by: Blue Swirl <blauwirbel@gmail.com> --- tcg/ppc/tcg-target.c | 18 +++++++++++------- tcg/ppc64/tcg-target.c | 16 +++++++++------- 2 files changed, 20 insertions(+), 14 deletions(-)