[PATCH 0/2] arm: Implement M-profile trapping on division by zero

[PATCH 0/2] arm: Implement M-profile trapping on division by zero

[Peter Maydell]

Unlike A-profile, for M-profile the UDIV and SDIV insns can be
configured to raise an exception on division by zero, using the CCR
DIV_0_TRP bit.  This patchset implements that missing functionality
by having the udiv and sdiv helpers raise an exception if needed.

Some questions:

Is it worth allowing A-profile to retain the mildly better codegen it
gets from not having to pass in 'env' and marking the helper as
no-side-effects (ie having M-specific udiv/sdiv helpers) ?

Is it worth inlining either udiv or sdiv for the A-profile case?
udiv can be done with movcond/movcond/divu, something like:

    /* t1 = (t2 == 0) ? 0 : t1;    t2 = (t2 == 0) ? 1 : t2 */
    tcg_gen_movcond_i32(TCG_COND_EQ, t1, t2, tcg_constant_i32(0),
    tcg_constant_i32(0), t1);
    tcg_gen_movcond_i32(TCG_COND_EQ, t2, t2, tcg_constant_i32(0),
    tcg_constant_i32(1), t2);
    /* Either t1 / t2; or 0 / 1 to give 0 for division-by-zero */
    tcg_gen_divu_i32(t1, t1, t2);

sdiv is more painful because it needs to check for both x/0 and
INTMIN/-1 cases.  Some other targets choose to generate inline TCG
ops for it, though.

Side note, I don't understand the x86-64 codegen for the above
sketch of an inline udiv. When I try it the TCG ops are

  mov_i32 tmp3,r2
  mov_i32 tmp6,r3
  movcond_i32 tmp3,tmp6,$0x0,$0x0,tmp3,eq
  movcond_i32 tmp6,tmp6,$0x0,$0x1,tmp6,eq
  mov_i32 tmp7,$0x0
  divu2_i32 tmp3,tmp7,tmp3,tmp7,tmp6
  mov_i32 r3,tmp3

but the x86 code is
0x7f5f1807dc0c:  45 33 f6                 xorl     %r14d, %r14d
0x7f5f1807dc0f:  45 85 ed                 testl    %r13d, %r13d
0x7f5f1807dc12:  45 0f 44 e6              cmovel   %r14d, %r12d
0x7f5f1807dc16:  41 bf 01 00 00 00        movl     $1, %r15d
0x7f5f1807dc1c:  45 3b ee                 cmpl     %r14d, %r13d
0x7f5f1807dc1f:  45 0f 44 ef              cmovel   %r15d, %r13d
0x7f5f1807dc23:  41 8b c4                 movl     %r12d, %eax
0x7f5f1807dc26:  41 8b d6                 movl     %r14d, %edx
0x7f5f1807dc29:  41 f7 f5                 divl     %r13d

where the comparison for the first cmovel is 'testl %r13d, %r13d",
but the second comparison is 'cmpl %r14d, %r13d'.  That's the same
effect (given r14 is 0) but I don't understand why the backend has
chosen to generate different code for the two cases.  (Ideally of
course it would notice that it already had generated the condition
check and not repeat it.)

thanks
-- PMM

Peter Maydell (2):
  target/arm: Re-indent sdiv and udiv helpers
  target/arm: Implement M-profile trapping on division by zero

 target/arm/cpu.h       |  1 +
 target/arm/helper.h    |  4 ++--
 target/arm/helper.c    | 34 ++++++++++++++++++++++++++--------
 target/arm/m_helper.c  |  4 ++++
 target/arm/translate.c |  4 ++--
 5 files changed, 35 insertions(+), 12 deletions(-)

-- 
2.20.1

[PATCH 1/2] target/arm: Re-indent sdiv and udiv helpers

[Peter Maydell]

We're about to make a code change to the sdiv and udiv helper
functions, so first fix their indentation and coding style.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 155d8bf2399..8e9c2a2cf8c 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -9355,17 +9355,20 @@ uint32_t HELPER(uxtb16)(uint32_t x)
 
 int32_t HELPER(sdiv)(int32_t num, int32_t den)
 {
-    if (den == 0)
-      return 0;
-    if (num == INT_MIN && den == -1)
-      return INT_MIN;
+    if (den == 0) {
+        return 0;
+    }
+    if (num == INT_MIN && den == -1) {
+        return INT_MIN;
+    }
     return num / den;
 }
 
 uint32_t HELPER(udiv)(uint32_t num, uint32_t den)
 {
-    if (den == 0)
-      return 0;
+    if (den == 0) {
+        return 0;
+    }
     return num / den;
 }
 
-- 
2.20.1

[PATCH 2/2] target/arm: Implement M-profile trapping on division by zero

[Peter Maydell]

Unlike A-profile, for M-profile the UDIV and SDIV insns can be
configured to raise an exception on division by zero, using the CCR
DIV_0_TRP bit.

Implement support for setting this bit by making the helper functions
raise the appropriate exception.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.h       |  1 +
 target/arm/helper.h    |  4 ++--
 target/arm/helper.c    | 19 +++++++++++++++++--
 target/arm/m_helper.c  |  4 ++++
 target/arm/translate.c |  4 ++--
 5 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 9f0a5f84d50..5cf8996ae3c 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -54,6 +54,7 @@
 #define EXCP_LAZYFP         20   /* v7M fault during lazy FP stacking */
 #define EXCP_LSERR          21   /* v8M LSERR SecureFault */
 #define EXCP_UNALIGNED      22   /* v7M UNALIGNED UsageFault */
+#define EXCP_DIVBYZERO      23   /* v7M DIVBYZERO UsageFault */
 /* NB: add new EXCP_ defines to the array in arm_log_exception() too */
 
 #define ARMV7M_EXCP_RESET   1
diff --git a/target/arm/helper.h b/target/arm/helper.h
index 248569b0cd8..aee8f0019b4 100644
--- a/target/arm/helper.h
+++ b/target/arm/helper.h
@@ -6,8 +6,8 @@ DEF_HELPER_3(add_saturate, i32, env, i32, i32)
 DEF_HELPER_3(sub_saturate, i32, env, i32, i32)
 DEF_HELPER_3(add_usaturate, i32, env, i32, i32)
 DEF_HELPER_3(sub_usaturate, i32, env, i32, i32)
-DEF_HELPER_FLAGS_2(sdiv, TCG_CALL_NO_RWG_SE, s32, s32, s32)
-DEF_HELPER_FLAGS_2(udiv, TCG_CALL_NO_RWG_SE, i32, i32, i32)
+DEF_HELPER_FLAGS_3(sdiv, TCG_CALL_NO_RWG, s32, env, s32, s32)
+DEF_HELPER_FLAGS_3(udiv, TCG_CALL_NO_RWG, i32, env, i32, i32)
 DEF_HELPER_FLAGS_1(rbit, TCG_CALL_NO_RWG_SE, i32, i32)
 
 #define PAS_OP(pfx)  \
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 8e9c2a2cf8c..56c520cf8e9 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -9345,6 +9345,18 @@ uint32_t HELPER(sxtb16)(uint32_t x)
     return res;
 }
 
+static void handle_possible_div0_trap(CPUARMState *env, uintptr_t ra)
+{
+    /*
+     * Take a division-by-zero exception if necessary; otherwise return
+     * to get the usual non-trapping division behaviour (result of 0)
+     */
+    if (arm_feature(env, ARM_FEATURE_M)
+        && (env->v7m.ccr[env->v7m.secure] & R_V7M_CCR_DIV_0_TRP_MASK)) {
+        raise_exception_ra(env, EXCP_DIVBYZERO, 0, 1, ra);
+    }
+}
+
 uint32_t HELPER(uxtb16)(uint32_t x)
 {
     uint32_t res;
@@ -9353,9 +9365,10 @@ uint32_t HELPER(uxtb16)(uint32_t x)
     return res;
 }
 
-int32_t HELPER(sdiv)(int32_t num, int32_t den)
+int32_t HELPER(sdiv)(CPUARMState *env, int32_t num, int32_t den)
 {
     if (den == 0) {
+        handle_possible_div0_trap(env, GETPC());
         return 0;
     }
     if (num == INT_MIN && den == -1) {
@@ -9364,9 +9377,10 @@ int32_t HELPER(sdiv)(int32_t num, int32_t den)
     return num / den;
 }
 
-uint32_t HELPER(udiv)(uint32_t num, uint32_t den)
+uint32_t HELPER(udiv)(CPUARMState *env, uint32_t num, uint32_t den)
 {
     if (den == 0) {
+        handle_possible_div0_trap(env, GETPC());
         return 0;
     }
     return num / den;
@@ -9567,6 +9581,7 @@ void arm_log_exception(int idx)
             [EXCP_LAZYFP] = "v7M exception during lazy FP stacking",
             [EXCP_LSERR] = "v8M LSERR UsageFault",
             [EXCP_UNALIGNED] = "v7M UNALIGNED UsageFault",
+            [EXCP_DIVBYZERO] = "v7M DIVBYZERO UsageFault",
         };
 
         if (idx >= 0 && idx < ARRAY_SIZE(excnames)) {
diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
index 20761c94877..47903b3dc35 100644
--- a/target/arm/m_helper.c
+++ b/target/arm/m_helper.c
@@ -2252,6 +2252,10 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE, env->v7m.secure);
         env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_UNALIGNED_MASK;
         break;
+    case EXCP_DIVBYZERO:
+        armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE, env->v7m.secure);
+        env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_DIVBYZERO_MASK;
+        break;
     case EXCP_SWI:
         /* The PC already points to the next instruction.  */
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_SVC, env->v7m.secure);
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 80c282669f0..28eabeb2323 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -7992,9 +7992,9 @@ static bool op_div(DisasContext *s, arg_rrr *a, bool u)
     t1 = load_reg(s, a->rn);
     t2 = load_reg(s, a->rm);
     if (u) {
-        gen_helper_udiv(t1, t1, t2);
+        gen_helper_udiv(t1, cpu_env, t1, t2);
     } else {
-        gen_helper_sdiv(t1, t1, t2);
+        gen_helper_sdiv(t1, cpu_env, t1, t2);
     }
     tcg_temp_free_i32(t2);
     store_reg(s, a->rd, t1);
-- 
2.20.1

Re: [PATCH 0/2] arm: Implement M-profile trapping on division by zero

[Richard Henderson]

On 7/30/21 5:16 AM, Peter Maydell wrote:
> Unlike A-profile, for M-profile the UDIV and SDIV insns can be
> configured to raise an exception on division by zero, using the CCR
> DIV_0_TRP bit.  This patchset implements that missing functionality
> by having the udiv and sdiv helpers raise an exception if needed.
> 
> Some questions:
> 
> Is it worth allowing A-profile to retain the mildly better codegen it
> gets from not having to pass in 'env' and marking the helper as
> no-side-effects (ie having M-specific udiv/sdiv helpers) ?

Probably not.

> Is it worth inlining either udiv or sdiv for the A-profile case?

Probably not.

>    mov_i32 tmp3,r2
>    mov_i32 tmp6,r3
>    movcond_i32 tmp3,tmp6,$0x0,$0x0,tmp3,eq
>    movcond_i32 tmp6,tmp6,$0x0,$0x1,tmp6,eq
>    mov_i32 tmp7,$0x0
>    divu2_i32 tmp3,tmp7,tmp3,tmp7,tmp6
>    mov_i32 r3,tmp3
> 
> but the x86 code is
> 0x7f5f1807dc0c:  45 33 f6                 xorl     %r14d, %r14d
> 0x7f5f1807dc0f:  45 85 ed                 testl    %r13d, %r13d
> 0x7f5f1807dc12:  45 0f 44 e6              cmovel   %r14d, %r12d


At the start of the first movcond, $0x0 is not allocated to a register, and the 
constraints allow a constant for argument 3.  Then, constraints do not allow a constant 
for argument 4 so we load $0x0 into a register.

> 0x7f5f1807dc16:  41 bf 01 00 00 00        movl     $1, %r15d
> 0x7f5f1807dc1c:  45 3b ee                 cmpl     %r14d, %r13d
> 0x7f5f1807dc1f:  45 0f 44 ef              cmovel   %r15d, %r13d

At the start of the second movcond, $0x0 is loaded into a register, so we use it.

> (Ideally of
> course it would notice that it already had generated the condition
> check and not repeat it.)

Yep.


r~

Re: [PATCH 1/2] target/arm: Re-indent sdiv and udiv helpers

[Richard Henderson]

On 7/30/21 5:16 AM, Peter Maydell wrote:
> We're about to make a code change to the sdiv and udiv helper
> functions, so first fix their indentation and coding style.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/helper.c | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 2/2] target/arm: Implement M-profile trapping on division by zero

[Richard Henderson]

On 7/30/21 5:16 AM, Peter Maydell wrote:
> Unlike A-profile, for M-profile the UDIV and SDIV insns can be
> configured to raise an exception on division by zero, using the CCR
> DIV_0_TRP bit.
> 
> Implement support for setting this bit by making the helper functions
> raise the appropriate exception.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/cpu.h       |  1 +
>   target/arm/helper.h    |  4 ++--
>   target/arm/helper.c    | 19 +++++++++++++++++--
>   target/arm/m_helper.c  |  4 ++++
>   target/arm/translate.c |  4 ++--
>   5 files changed, 26 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~