From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33443) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLYbV-0008LK-DM for qemu-devel@nongnu.org; Tue, 19 Jan 2016 10:53:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aLYbS-0002dn-3u for qemu-devel@nongnu.org; Tue, 19 Jan 2016 10:53:57 -0500 Received: from mail-ig0-x244.google.com ([2607:f8b0:4001:c05::244]:35510) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLYbR-0002d9-QZ for qemu-devel@nongnu.org; Tue, 19 Jan 2016 10:53:54 -0500 Received: by mail-ig0-x244.google.com with SMTP id mw1so9687417igb.2 for ; Tue, 19 Jan 2016 07:53:53 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <1452268394-31252-3-git-send-email-alex.bennee@linaro.org> References: <1452268394-31252-1-git-send-email-alex.bennee@linaro.org> <1452268394-31252-3-git-send-email-alex.bennee@linaro.org> Date: Tue, 19 Jan 2016 16:53:52 +0100 Message-ID: From: alvise rigo Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC PATCH 2/2] softmmu: simplify helper_*_st_name with smmu_helper(do_unl_store) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?QWxleCBCZW5uw6ll?= Cc: Peter Crosthwaite , Claudio Fontana , QEMU Developers , Paolo Bonzini , Jani Kokkonen , Richard Henderson On Fri, Jan 8, 2016 at 4:53 PM, Alex Benn=C3=A9e w= rote: > From: Alvise Rigo > > Attempting to simplify the helper_*_st_name, wrap the > do_unaligned_access code into an shared inline function. As this also > removes the goto statement the inline code is expanded twice in each > helper. > > Suggested-by: Jani Kokkonen > Suggested-by: Claudio Fontana > CC: Alvise Rigo > Signed-off-by: Alex Benn=C3=A9e > > --- > v2 > - based on original patch from Alvise > - uses a single shared inline function to reduce duplication > --- > softmmu_template.h | 75 ++++++++++++++++++++++++++++--------------------= ------ > 1 file changed, 39 insertions(+), 36 deletions(-) > > diff --git a/softmmu_template.h b/softmmu_template.h > index 0074bd7..ac0b4ac 100644 > --- a/softmmu_template.h > +++ b/softmmu_template.h > @@ -159,6 +159,39 @@ static inline int smmu_helper(victim_tlb_hit) (const= bool is_read, CPUArchState > } > > #ifndef SOFTMMU_CODE_ACCESS > + > +static inline void smmu_helper(do_unl_store)(CPUArchState *env, > + bool little_endian, > + DATA_TYPE val, > + target_ulong addr, > + TCGMemOpIdx oi, > + unsigned mmu_idx, > + uintptr_t retaddr) > +{ > + int i; > + > + if ((get_memop(oi) & MO_AMASK) =3D=3D MO_ALIGN) { > + cpu_unaligned_access(ENV_GET_CPU(env), addr, MMU_DATA_STORE, > + mmu_idx, retaddr); > + } > + /* Note: relies on the fact that tlb_fill() does not remove the > + * previous page from the TLB cache. */ > + for (i =3D DATA_SIZE - 1; i >=3D 0; i--) { > + uint8_t val8; > + if (little_endian) { > + /* Little-endian extract. */ > + val8 =3D val >> (i * 8); > + } else { > + /* Big-endian extract. */ > + val8 =3D val >> (((DATA_SIZE - 1) * 8) - (i * 8)); > + } > + /* Note the adjustment at the beginning of the function. > + Undo that for the recursion. */ > + glue(helper_ret_stb, MMUSUFFIX)(env, addr + i, val8, > + oi, retaddr + GETPC_ADJ); > + } > +} > + > static inline DATA_TYPE glue(io_read, SUFFIX)(CPUArchState *env, > CPUIOTLBEntry *iotlbentry, > target_ulong addr, > @@ -416,7 +449,8 @@ void helper_le_st_name(CPUArchState *env, target_ulon= g addr, DATA_TYPE val, > if (unlikely(tlb_addr & ~TARGET_PAGE_MASK)) { > CPUIOTLBEntry *iotlbentry; > if ((addr & (DATA_SIZE - 1)) !=3D 0) { > - goto do_unaligned_access; > + smmu_helper(do_unl_store)(env, true, val, addr, oi, mmu_idx,= retaddr); > + return; > } > iotlbentry =3D &env->iotlb[mmu_idx][index]; > > @@ -431,23 +465,7 @@ void helper_le_st_name(CPUArchState *env, target_ulo= ng addr, DATA_TYPE val, > if (DATA_SIZE > 1 > && unlikely((addr & ~TARGET_PAGE_MASK) + DATA_SIZE - 1 > >=3D TARGET_PAGE_SIZE)) { > - int i; > - do_unaligned_access: > - if ((get_memop(oi) & MO_AMASK) =3D=3D MO_ALIGN) { > - cpu_unaligned_access(ENV_GET_CPU(env), addr, MMU_DATA_STORE, > - mmu_idx, retaddr); > - } > - /* XXX: not efficient, but simple */ > - /* Note: relies on the fact that tlb_fill() does not remove the > - * previous page from the TLB cache. */ > - for (i =3D DATA_SIZE - 1; i >=3D 0; i--) { > - /* Little-endian extract. */ > - uint8_t val8 =3D val >> (i * 8); > - /* Note the adjustment at the beginning of the function. > - Undo that for the recursion. */ > - glue(helper_ret_stb, MMUSUFFIX)(env, addr + i, val8, > - oi, retaddr + GETPC_ADJ); > - } > + smmu_helper(do_unl_store)(env, true, val, addr, oi, mmu_idx, ret= addr); > return; > } > > @@ -496,7 +514,8 @@ void helper_be_st_name(CPUArchState *env, target_ulon= g addr, DATA_TYPE val, > if (unlikely(tlb_addr & ~TARGET_PAGE_MASK)) { > CPUIOTLBEntry *iotlbentry; > if ((addr & (DATA_SIZE - 1)) !=3D 0) { > - goto do_unaligned_access; > + smmu_helper(do_unl_store)(env, false, val, addr, oi, mmu_idx= , retaddr); > + return; > } > iotlbentry =3D &env->iotlb[mmu_idx][index]; > > @@ -511,23 +530,7 @@ void helper_be_st_name(CPUArchState *env, target_ulo= ng addr, DATA_TYPE val, > if (DATA_SIZE > 1 > && unlikely((addr & ~TARGET_PAGE_MASK) + DATA_SIZE - 1 > >=3D TARGET_PAGE_SIZE)) { > - int i; > - do_unaligned_access: > - if ((get_memop(oi) & MO_AMASK) =3D=3D MO_ALIGN) { > - cpu_unaligned_access(ENV_GET_CPU(env), addr, MMU_DATA_STORE, > - mmu_idx, retaddr); > - } > - /* XXX: not efficient, but simple */ > - /* Note: relies on the fact that tlb_fill() does not remove the > - * previous page from the TLB cache. */ > - for (i =3D DATA_SIZE - 1; i >=3D 0; i--) { > - /* Big-endian extract. */ > - uint8_t val8 =3D val >> (((DATA_SIZE - 1) * 8) - (i * 8)); > - /* Note the adjustment at the beginning of the function. > - Undo that for the recursion. */ > - glue(helper_ret_stb, MMUSUFFIX)(env, addr + i, val8, > - oi, retaddr + GETPC_ADJ); > - } > + smmu_helper(do_unl_store)(env, false, val, addr, oi, mmu_idx, re= taddr); > return; > } > > -- > 2.6.4 > This approach makes sense to me, given that the leg of the *if* statement is actually inlined depending on the (constant) value of little_endian. The thing not convincing me is the fact that we are not imposing the inlining, but we are relying on the compiler optimizations to do it. I wonder if this will always happen, even with other compilers (clang). alvise