Re: [Qemu-devel] [PATCH 07/10] target-avr: adding instruction decoder

2016-06-04 Thread Michael Rolnik 
1. no more custings.
2. yes, it is generated code. I would like to commit the generator. however
it is not very ripe yet. and it has a dependency on yaml-cpp and xsltproc.
3. the generator does not assume that same bits are extracted. it will be
fixed later.
4. 0 is no longer returned.

On Sun, Jun 5, 2016 at 2:00 AM, Richard Henderson  wrote:

> On 06/02/2016 01:06 PM, Michael Rolnik wrote:
>
>> +uint32_tavr_decode(uint32_t pc, uint32_t *length, uint32_t code,
>> translate_function_t *translate)
>> +{
>> +uint32_topcode  = extract32(code, 0, 16);
>> +switch (opcode & 0xd000) {
>> +case0x: {
>> +uint32_topcode  = extract32(code, 0, 16);
>> +switch (opcode & 0x2c00) {
>> +case0x: {
>> +uint32_topcode  = extract32(code, 0, 16);
>>
>
> Why do you keep extracting the same value into variables that shadow each
> other?
>
> I can only assume that this is machine-generated code.  Is there any
> reason that you want to commit the quite unreadable generated code instead
> of the original input and generator?
>
> +*translate =
>> (translate_function_t)_translate_NOP;
>>
>
> The casts are, in this and other cases, unnecessary, and could easily mask
> a real problem.
>
> +return  0;
>>
>
> Since you only ever return 0, what's the point of the return value?
>
>
> r~
>
>


-- 
Best Regards,
Michael Rolnik

Re: [Qemu-devel] [PATCH 07/10] target-avr: adding instruction decoder

2016-06-04 Thread Richard Henderson 

On 06/02/2016 01:06 PM, Michael Rolnik wrote:

+uint32_tavr_decode(uint32_t pc, uint32_t *length, uint32_t code, 
translate_function_t *translate)
+{
+uint32_topcode  = extract32(code, 0, 16);
+switch (opcode & 0xd000) {
+case0x: {
+uint32_topcode  = extract32(code, 0, 16);
+switch (opcode & 0x2c00) {
+case0x: {
+uint32_topcode  = extract32(code, 0, 16);


Why do you keep extracting the same value into variables that shadow each other?

I can only assume that this is machine-generated code.  Is there any reason 
that you want to commit the quite unreadable generated code instead of the 
original input and generator?



+*translate = 
(translate_function_t)_translate_NOP;


The casts are, in this and other cases, unnecessary, and could easily mask a 
real problem.



+return  0;


Since you only ever return 0, what's the point of the return value?


r~

[Qemu-devel] [PATCH 07/10] target-avr: adding instruction decoder

2016-06-02 Thread Michael Rolnik 
Signed-off-by: Michael Rolnik 
---
 target-avr/decode.c | 724 
 1 file changed, 724 insertions(+)
 create mode 100644 target-avr/decode.c

diff --git a/target-avr/decode.c b/target-avr/decode.c
new file mode 100644
index 000..22e2d36
--- /dev/null
+++ b/target-avr/decode.c
@@ -0,0 +1,724 @@
+/*
+ * QEMU AVR CPU
+ *
+ * Copyright (c) 2016 Michael Rolnik
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see
+ * 
+ */
+
+
+#include 
+#include "translate.h"
+
+
+uint32_tavr_decode(uint32_t pc, uint32_t *length, uint32_t code, 
translate_function_t *translate)
+{
+uint32_topcode  = extract32(code, 0, 16);
+switch (opcode & 0xd000) {
+case0x: {
+uint32_topcode  = extract32(code, 0, 16);
+switch (opcode & 0x2c00) {
+case0x: {
+uint32_topcode  = extract32(code, 0, 16);
+switch (opcode & 0x0300) {
+case0x: {
+*length = 16;
+*translate = 
(translate_function_t)_translate_NOP;
+break;
+}
+case0x0100: {
+*length = 16;
+*translate = 
(translate_function_t)_translate_MOVW;
+break;
+}
+case0x0200: {
+*length = 16;
+*translate = 
(translate_function_t)_translate_MULS;
+break;
+}
+case0x0300: {
+uint32_topcode  = extract32(code, 0, 16);
+switch (opcode & 0x0088) {
+case0x: {
+*length = 16;
+*translate = 
(translate_function_t)_translate_MULSU;
+break;
+}
+case0x0008: {
+*length = 16;
+*translate = 
(translate_function_t)_translate_FMUL;
+break;
+}
+case0x0080: {
+*length = 16;
+*translate = 
(translate_function_t)_translate_FMULS;
+break;
+}
+case0x0088: {
+*length = 16;
+*translate = 
(translate_function_t)_translate_FMULSU;
+break;
+}
+}
+break;
+}
+}
+break;
+}
+case0x0400: {
+*length = 16;
+*translate = (translate_function_t)_translate_CPC;
+break;
+}
+case0x0800: {
+*length = 16;
+*translate = (translate_function_t)_translate_SBC;
+break;
+}
+case0x0c00: {
+*length = 16;
+*translate = (translate_function_t)_translate_ADD;
+break;
+}
+case0x2000: {
+*length = 16;
+*translate = (translate_function_t)_translate_AND;
+break;
+}
+case0x2400: {
+*length = 16;
+*translate = (translate_function_t)_translate_EOR;
+break;
+}
+case0x2800: {
+*length = 16;
+*translate = (translate_function_t)_translate_OR;
+break;
+}
+case

[Qemu-devel] [PATCH 07/10] target-avr: adding instruction decoder

2016-06-02 Thread Michael Rolnik 
Signed-off-by: Michael Rolnik 
---
 target-avr/decode.c | 724 
 1 file changed, 724 insertions(+)
 create mode 100644 target-avr/decode.c

diff --git a/target-avr/decode.c b/target-avr/decode.c
new file mode 100644
index 000..22e2d36
--- /dev/null
+++ b/target-avr/decode.c
@@ -0,0 +1,724 @@
+/*
+ * QEMU AVR CPU
+ *
+ * Copyright (c) 2016 Michael Rolnik
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see
+ * 
+ */
+
+
+#include 
+#include "translate.h"
+
+
+uint32_tavr_decode(uint32_t pc, uint32_t *length, uint32_t code, 
translate_function_t *translate)
+{
+uint32_topcode  = extract32(code, 0, 16);
+switch (opcode & 0xd000) {
+case0x: {
+uint32_topcode  = extract32(code, 0, 16);
+switch (opcode & 0x2c00) {
+case0x: {
+uint32_topcode  = extract32(code, 0, 16);
+switch (opcode & 0x0300) {
+case0x: {
+*length = 16;
+*translate = 
(translate_function_t)_translate_NOP;
+break;
+}
+case0x0100: {
+*length = 16;
+*translate = 
(translate_function_t)_translate_MOVW;
+break;
+}
+case0x0200: {
+*length = 16;
+*translate = 
(translate_function_t)_translate_MULS;
+break;
+}
+case0x0300: {
+uint32_topcode  = extract32(code, 0, 16);
+switch (opcode & 0x0088) {
+case0x: {
+*length = 16;
+*translate = 
(translate_function_t)_translate_MULSU;
+break;
+}
+case0x0008: {
+*length = 16;
+*translate = 
(translate_function_t)_translate_FMUL;
+break;
+}
+case0x0080: {
+*length = 16;
+*translate = 
(translate_function_t)_translate_FMULS;
+break;
+}
+case0x0088: {
+*length = 16;
+*translate = 
(translate_function_t)_translate_FMULSU;
+break;
+}
+}
+break;
+}
+}
+break;
+}
+case0x0400: {
+*length = 16;
+*translate = (translate_function_t)_translate_CPC;
+break;
+}
+case0x0800: {
+*length = 16;
+*translate = (translate_function_t)_translate_SBC;
+break;
+}
+case0x0c00: {
+*length = 16;
+*translate = (translate_function_t)_translate_ADD;
+break;
+}
+case0x2000: {
+*length = 16;
+*translate = (translate_function_t)_translate_AND;
+break;
+}
+case0x2400: {
+*length = 16;
+*translate = (translate_function_t)_translate_EOR;
+break;
+}
+case0x2800: {
+*length = 16;
+*translate = (translate_function_t)_translate_OR;
+break;
+}
+case