linux-mips
[Top] [All Lists]

Re: [PATCH] [RFC] Proposed changes to eliminate 'union mips_instruction'

To: "Steven J. Hill" <sjhill@mips.com>
Subject: Re: [PATCH] [RFC] Proposed changes to eliminate 'union mips_instruction' type.
From: David Daney <ddaney.cavm@gmail.com>
Date: Tue, 15 Jan 2013 11:41:04 -0800
Cc: linux-mips@linux-mips.org, ralf@linux-mips.org, cernekee@gmail.com, kevink@paralogos.com
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=x-received:message-id:date:from:user-agent:mime-version:to:cc :subject:references:in-reply-to:content-type :content-transfer-encoding; bh=AHnlWPAyEij0BReHwacG16bM6bZMCMls3qVspKTG5LY=; b=zwR8z0TKK48f7mz/Bc2nNUVY2HMQHlW4EN1ROFXB7YjrXWNP6K7BKjPwO4vaEXZify lZXrzxWs16zs+I6hItr+4TX5R9G/z/OBsjkgZzTibyrj9MC4QsVj/bBXpV4mSWvWOKmP HAdvqzvnQUo5061ahXU0WVieqkAfoePrSrGnkYJps9oGwm6RR70UvtHnsARvDeV3BkVm XsnepsLINKMvtzc2CvB9ldTwPdhwrCXwgooWnYHc2iBbk6hXCkMyv6ZtAXMQ0V0qJ8EW Lm3Ax3YYDNK/Gb3X4MeH+P3Eojjjsb9iu1yb+eVQndgmQFy3BliI9BbnXUHzfTfDKjB3 OLqg==
In-reply-to: <1358230420-3575-1-git-send-email-sjhill@mips.com>
List-archive: <http://www.linux-mips.org/archives/linux-mips/>
List-help: <mailto:ecartis@linux-mips.org?Subject=help>
List-id: linux-mips <linux-mips.eddie.linux-mips.org>
List-owner: <mailto:ralf@linux-mips.org>
List-post: <mailto:linux-mips@linux-mips.org>
List-software: Ecartis version 1.0.0
List-subscribe: <mailto:ecartis@linux-mips.org?subject=subscribe%20linux-mips>
List-unsubscribe: <mailto:ecartis@linux-mips.org?subject=unsubscribe%20linux-mips>
References: <1358230420-3575-1-git-send-email-sjhill@mips.com>
Sender: linux-mips-bounce@linux-mips.org
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Thunderbird/17.0
On 01/14/2013 10:13 PM, Steven J. Hill wrote:
From: "Steven J. Hill" <sjhill@mips.com>

This patch shows the use of macros in place of 'union mips_instruction'
type.

Why?  What are the benefits of doing this?

I converted all usages of 'j_format' and 'r_format' to show how
the code and macros could look and be defined. I have tested these
changes on big and little endian platforms.

I want input from everyone, please!!! I want consensus on the macro
definitions, placement of parenthesis for them, spacing in the header
file, etc. This is your chance to be completely anal and have fun
arguments over how things should be. I would also like input on how
the maintainers would like the patchsets to look like. For example:

   [1/X] - Convert 'j_format'
   [2/X] - Convert 'r_format'
   [3/X] - Convert 'f_format'
   [4/X] - Convert 'u_format'
   ...
   [X/X] - Remove usage of 'union mips_instruction' type completely.

Also, I noticed 'p_format' is not used anywhere. Can we kill it? Be
picky and help with this conversion. Thanks.

Signed-off-by: Steven J. Hill <sjhill@mips.com>
---
  arch/mips/include/asm/inst.h   |   66 +++++++++++-----------------------------
  arch/mips/kernel/branch.c      |   13 ++++----
  arch/mips/kernel/jump_label.c  |   10 +++---
  arch/mips/kernel/kgdb.c        |   10 ++----
  arch/mips/kernel/kprobes.c     |   18 +++++------
  arch/mips/kernel/process.c     |   10 +++---
  arch/mips/oprofile/backtrace.c |    2 +-
  7 files changed, 46 insertions(+), 83 deletions(-)

diff --git a/arch/mips/include/asm/inst.h b/arch/mips/include/asm/inst.h
index ab84064..856b14e 100644
--- a/arch/mips/include/asm/inst.h
+++ b/arch/mips/include/asm/inst.h
@@ -192,15 +192,27 @@ enum lx_func {
        lbx_op  = 0x16,
  };

+#define INSN_OPCODE(insn)              (insn >> 26)
+#define INSN_RS(insn)                  ((insn >> 21) & 0x1f)
+#define INSN_RT(insn)                  ((insn >> 16) & 0x1f)
+#define INSN_RD(insn)                  ((insn >> 11) & 0x1f)
+#define INSN_RE(insn)                  ((insn >> 6) & 0x1f)
+#define INSN_FUNC(insn)                        (insn & 0x0000003f)
+
+#define J_INSN(op,target)              ((op << 26) | target)

What is the type of J_INSN()? What happens if target overflows into the 'op' field?


+#define J_INSN_TARGET(insn)            (insn & 0x03ffffff)
+#define R_INSN(op,rs,rt,rd,re,func)    ((op << 26) | (rs << 21) |  \
+                                        (rt << 16) | (rd << 11) |  \
+                                        (re << 6) | func)
+#define F_INSN(op,fmt,rt,rd,re,func)   R_INSN(op,fmt,rt,rd,re,func)
+#define F_INSN_FMT(insn)               INSN_RS(insn)
+#define U_INSN(op,rs,uimm)             ((op << 26) | (rs << 21) | uimmediate)
[...]
+       unsigned int n_insn = insn.word;

I don't like that the width of an insn is not obvious by looking at the code.

Can we, assuming we merge something like this, make it something like u32, or insn_t? I'm not sure which is better.


[...]

David Daney

<Prev in Thread] Current Thread [Next in Thread>