From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39709) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YQIGZ-0007AS-Hz for qemu-devel@nongnu.org; Tue, 24 Feb 2015 11:23:24 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YQIGW-0006pU-9g for qemu-devel@nongnu.org; Tue, 24 Feb 2015 11:23:23 -0500 Received: from out1134-185.mail.aliyun.com ([42.120.134.185]:11634) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YQIGV-0006pA-ML for qemu-devel@nongnu.org; Tue, 24 Feb 2015 11:23:20 -0500 Message-ID: <54ECA746.8030804@sunrus.com.cn> Date: Wed, 25 Feb 2015 00:31:02 +0800 From: Chen Gang S MIME-Version: 1.0 References: <54EC2DEE.8050809@sunrus.com.cn> <54EC88D6.3060402@ezchip.com> In-Reply-To: <54EC88D6.3060402@ezchip.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] target-tilegx: Execute _start and reach to __libc_start_main successfully List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Chris Metcalf , Peter Maydell , Richard Henderson , "walt@tilera.com" , Riku Voipio Cc: qemu-devel On 2/24/15 22:21, Chris Metcalf wrote: > On 2/24/2015 2:53 AM, Chen Gang S wrote: >> typedef struct CPUTLState { >> + uint64_t regs[TILEGX_R_COUNT]; /* Common used registers by outside */ >> + uint64_t zero; /* Zero register */ >> + uint64_t pc; /* Current pc */ >> CPU_COMMON >> } CPUTLState; > > I skimmed through this and my only comment is that I was surprised to see "zero" as part of the state, since it's always 0. :-) No doubt there is some reason that this makes sense. > Originally, I did not add zero register, but after think of, for gen_st operation, tcg_gen_st*() only support from tcg_target_long to register, so I add zero register for "offsetof(CPUTLState, zero)". Welcome any better ways for it. >> +#define TILEGX_GEN_CHK_BEGIN(x) \ >> + if ((x) == TILEGX_R_ZERO) { >> +#define TILEGX_GEN_CHK_END(x) \ >> + return 0; \ >> + } \ >> + if ((x) >= TILEGX_R_COUNT) { \ >> + return -1; \ >> + } > > This macro pattern seems potentially a little confusing and I do wonder if there is some way to avoid having to explicitly check the zero register every time; for example, maybe you make it a legitimate part of the state and declare that there are 64 registers, and then just always finish any register-update phase by re-zeroing that register? It might yield a smaller code footprint and probably be just as fast, as long as it was easy to know where registers were updated. > Originally, I really used 64 registers, but after tried, I found that I still had to process TILEGX_R_ZERO specially, e.g. - When src is zero, we can use mov operation instead of add operation. - When dst is zero, in most cases, we can just skip the insn, but in some cases, it may cause exception in user mode (e.g st zero r0). Originally, I also tried a wrap function for zero register, but I found for different operands, when they meet zero register, they would need different operations: - For add/addi operation, it will change to mov/movi operation. - For mov operation, it will change to movi operation. - For shl3add, if srcb is zero register, it will change to shli operation. - For ld/st operation, it still be ld/st operation, but they need "offsetof(CPUTLState, zero)", and in some cases, it should be cause exception. So after think of, for me, I still prefer to use 56 registers with individual zero register, and use macros for it. > Also, note that you should model accesses to registers 56..62 as causing an interrupt to be raised, rather than returning -1. But you might choose to just put this on your TODO list and continue making forward progress for the time being. But a FIXME comment here would be appropriate. > Yeah, thanks. I shall add it when I send patch v2 for it. Also I forgot to use "offsetof(CPUTLState, zero)" for ld zero register case, and still "return 0" for "st zero r1" or "ld r1 zero". I shall fix it in the patch v2. >> + case 0x3800000000000000ULL: > > There are a lot of constants defined in the tile header, and presumably you could synthesize these constant values from them. Perhaps it would make sense to import that header into qemu and then use symbolic values for all of this kind of thing? > OK, thanks. originally I wanted to reuse them, but after think of, I prefer the 64-bit immediate number instead of. - The decoding function may be very long, but it is still a simple function, I guess, it is still simple enough for readers. - 64-bit immediate number has better performance under 64-bit machine (e.g x86-64). But I guess, there are still quite a few of inline functions (e.g. get src/dst) in arch/opcode_tilegx.h may be reused by me in the future. :-) > I can't really comment on most of the rest of the code, and I only skimmed it quickly, but generally: good work getting as far as you have! > Thank you for your work and your encouragement. Thanks. -- Chen Gang Open, share, and attitude like air, water, and life which God blessed