Skip to contentSkip to conversation

Support saturation with shift=0.

PATCH Applied Christophe Lyon 3 participants 5 messages Jan 19, 2011 – Jan 26, 2011

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Support saturation with shift=0.

This patch fixes corner-case saturations, when the target range is zero. It merely removes the guard against (sh == 0), and makes:
__ssat(0x87654321, 1) return 0xffffffff and set the saturation flag __usat(0x87654321, 0) return 0 and set the saturation flag

---
 target-arm/translate.c |   28 ++++++++++++----------------
 1 files changed, 12 insertions(+), 16 deletions(-)
target-arm/translate.c +12 -16
index 721bada..41cbb96 100644
@@ -6896,27 +6896,23 @@ static void disas_arm_insn(CPUState * env, DisasContext *s)
                             tcg_gen_shli_i32(tmp, tmp, shift);
                         }
                         sh = (insn >> 16) & 0x1f;
-                        if (sh != 0) {
-                            tmp2 = tcg_const_i32(sh);
-                            if (insn & (1 << 22))
-                                gen_helper_usat(tmp, tmp, tmp2);
-                            else
-                                gen_helper_ssat(tmp, tmp, tmp2);
-                            tcg_temp_free_i32(tmp2);
-                        }
+                        tmp2 = tcg_const_i32(sh);
+                        if (insn & (1 << 22))
+                          gen_helper_usat(tmp, tmp, tmp2);
+                        else
+                          gen_helper_ssat(tmp, tmp, tmp2);
+                        tcg_temp_free_i32(tmp2);
                         store_reg(s, rd, tmp);
                     } else if ((insn & 0x00300fe0) == 0x00200f20) {
                         /* [us]sat16 */
                         tmp = load_reg(s, rm);
                         sh = (insn >> 16) & 0x1f;
-                        if (sh != 0) {
-                            tmp2 = tcg_const_i32(sh);
-                            if (insn & (1 << 22))
-                                gen_helper_usat16(tmp, tmp, tmp2);
-                            else
-                                gen_helper_ssat16(tmp, tmp, tmp2);
-                            tcg_temp_free_i32(tmp2);
-                        }
+                        tmp2 = tcg_const_i32(sh);
+                        if (insn & (1 << 22))
+                          gen_helper_usat16(tmp, tmp, tmp2);
+                        else
+                          gen_helper_ssat16(tmp, tmp, tmp2);
+                        tcg_temp_free_i32(tmp2);
                         store_reg(s, rd, tmp);
                     } else if ((insn & 0x00700fe0) == 0x00000fa0) {
                         /* Select bytes.  */
Signature · 1 line
1.7.2.3
Reviewed-by on v1 1/1
#1 lore raw
Quoted 4 lines · Christophe Lyon

This patch fixes corner-case saturations, when the target range is zero. It merely removes the guard against (sh == 0), and makes:
__ssat(0x87654321, 1) return 0xffffffff and set the saturation flag

did you mean __ssat(0x87654321, 0) here? (they give the same result, of course, but it's the sh==0 case the patch is changing...)

Quoted 3 lines

__usat(0x87654321, 0) return 0 and set the saturation flag

Checked against the ARM ARM and tested by
random-instruction-sequence generation.

(We could have taken the opportunity of adding braces to conform to the coding style while touching these lines, but since the patch isn't changing them, just reindenting, I'm happy not to worry about this.)

(for the content of the patch if not the commit message, anyway).

-- PMM

#2 lore raw
Quoted 8 lines · Peter Maydell
Quoted 4 lines · Christophe Lyon

This patch fixes corner-case saturations, when the target range is zero. It merely removes the guard against (sh == 0), and makes:
__ssat(0x87654321, 1) return 0xffffffff and set the saturation flag

did you mean __ssat(0x87654321, 0) here? (they give the same result, of course, but it's the sh==0 case the patch is changing...)

Well... the ARM ARM says that the position for saturation is in the range 1 to 32, so I think the assembler encodes 1 less than what the user actually wrote. Hence at user level we use '1', but '0' is encoded and then parsed by qemu. Am I wrong?

Obviously, I can rephrase the commit message :-)

Quoted 8 lines
Quoted 4 lines

__usat(0x87654321, 0) return 0 and set the saturation flag

Checked against the ARM ARM and tested by
random-instruction-sequence generation.

Thanks.

#3 lore raw
Quoted 7 lines · Christophe Lyon
Quoted 6 lines · Peter Maydell
Quoted 4 lines · Christophe Lyon

This patch fixes corner-case saturations, when the target range is zero. It merely removes the guard against (sh == 0), and makes:
__ssat(0x87654321, 1) return 0xffffffff and set the saturation flag

Quoted 7 lines
Quoted 2 lines

did you mean __ssat(0x87654321, 0) here? (they give the same result, of course, but it's the sh==0 case the patch is changing...)

Well... the ARM ARM says that the position for saturation is in the range 1 to 32, so I think the assembler encodes 1 less than what the user actually wrote. Hence at user level we use '1', but '0' is encoded and then parsed by qemu. Am I wrong?

No, you're right, there's a "+1" in the SSAT decoding instructions which I hadn't noticed.

-- PMM

#4 lore raw
In reply to Christophe Lyon
Quoted 10 lines · Christophe Lyon

This patch fixes corner-case saturations, when the target range is zero. It merely removes the guard against (sh == 0), and makes:
__ssat(0x87654321, 1) return 0xffffffff and set the saturation flag __usat(0x87654321, 0) return 0 and set the saturation flag

---
 target-arm/translate.c |   28 ++++++++++++----------------
 1 files changed, 12 insertions(+), 16 deletions(-)

Thanks, applied.

Quoted 49 lines
target-arm/translate.c +12 -16
index 721bada..41cbb96 100644
@@ -6896,27 +6896,23 @@ static void disas_arm_insn(CPUState * env, DisasContext *s)
                             tcg_gen_shli_i32(tmp, tmp, shift);
                         }
                         sh = (insn >> 16) & 0x1f;
-                        if (sh != 0) {
-                            tmp2 = tcg_const_i32(sh);
-                            if (insn & (1 << 22))
-                                gen_helper_usat(tmp, tmp, tmp2);
-                            else
-                                gen_helper_ssat(tmp, tmp, tmp2);
-                            tcg_temp_free_i32(tmp2);
-                        }
+                        tmp2 = tcg_const_i32(sh);
+                        if (insn & (1 << 22))
+                          gen_helper_usat(tmp, tmp, tmp2);
+                        else
+                          gen_helper_ssat(tmp, tmp, tmp2);
+                        tcg_temp_free_i32(tmp2);
                         store_reg(s, rd, tmp);
                     } else if ((insn & 0x00300fe0) == 0x00200f20) {
                         /* [us]sat16 */
                         tmp = load_reg(s, rm);
                         sh = (insn >> 16) & 0x1f;
-                        if (sh != 0) {
-                            tmp2 = tcg_const_i32(sh);
-                            if (insn & (1 << 22))
-                                gen_helper_usat16(tmp, tmp, tmp2);
-                            else
-                                gen_helper_ssat16(tmp, tmp, tmp2);
-                            tcg_temp_free_i32(tmp2);
-                        }
+                        tmp2 = tcg_const_i32(sh);
+                        if (insn & (1 << 22))
+                          gen_helper_usat16(tmp, tmp, tmp2);
+                        else
+                          gen_helper_ssat16(tmp, tmp, tmp2);
+                        tcg_temp_free_i32(tmp2);
                         store_reg(s, rd, tmp);
                     } else if ((insn & 0x00700fe0) == 0x00000fa0) {
                         /* Select bytes.  */
Signature · 1 line
1.7.2.3
Signature · 2 lines
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
[email protected]                 http://www.aurel32.net