Re: [PATCH v6 26/61] target/riscv: vector single-width fractional multiply with rounding and saturation

[LIU Zhiwei]

On 2020/3/28 9:08, Richard Henderson wrote:

On 3/17/20 8:06 AM, LIU Zhiwei wrote:

+static int64_t vsmul64(CPURISCVState *env, int vxrm, int64_t a, int64_t b)
+{
+    uint8_t round;
+    uint64_t hi_64, lo_64, Hi62;
+    uint8_t hi62, hi63, lo63;
+
+    muls64(&lo_64, &hi_64, a, b);
+    hi62 = extract64(hi_64, 62, 1);
+    lo63 = extract64(lo_64, 63, 1);
+    hi63 = extract64(hi_64, 63, 1);
+    Hi62 = extract64(hi_64, 0, 62);

This seems like way more work than necessary.

+    if (hi62 != hi63) {
+        env->vxsat = 0x1;
+        return INT64_MAX;
+    }

This can only happen for a == b == INT64_MIN.
Perhaps just test exactly that and move it above the multiply?

+    round = get_round(vxrm, lo_64, 63);
+    if (round && (Hi62 == 0x3fffffff) && lo63) {
+        env->vxsat = 0x1;
+        return hi62 ? INT64_MIN : INT64_MAX;
+    } else {
+        if (lo63 && round) {
+            return (hi_64 + 1) << 1;
+        } else {
+            return (hi_64 << 1) | lo63 | round;
+        }
+    }

Yes, it's an error here. As INT64_MIN will never be  a right result.

   /* Cannot overflow, as there are always
      2 sign bits after multiply. */
   ret = (hi_64 << 1) | (lo_64 >> 63);
   if (round) {
       if (ret == INT64_MAX) {
           env->vxsat = 1;
       } else {
           ret += 1;
       }
   }
   return ret;

Thanks. I think it's  right and much clearer.

Zhiwei.

r~