diff mbox series

[committed,PR,rtl-optimization/115876,PR,rtl-optimization/115916] Fix sign/carry bit handling in ext-dce

Message ID fd054faa-87be-4fa1-826b-827fa11a1ec1@gmail.com
State New
Headers show
Series [committed,PR,rtl-optimization/115876,PR,rtl-optimization/115916] Fix sign/carry bit handling in ext-dce | expand

Commit Message

Jeff Law July 15, 2024, 11:06 p.m. UTC 
My change to fix a ubsan issue broke handling propagation of the 
carry/sign bit down through a right shift.  Thanks to Andreas for the 
analysis and proposed fix and Sergei for the testcase.

Bootstrapped and regression tested on x86-64.  Pushing to the trunk.

Jeff
commit 94b21f13763638f64e83e7f9959c7f1523b9eaed
Author: Jeff Law <jlaw@ventanamicro.com>
Date:   Mon Jul 15 16:57:44 2024 -0600

    Fix sign/carry bit handling in ext-dce.
    
    My change to fix a ubsan issue broke handling propagation of the carry/sign bit
    down through a right shift.  Thanks to Andreas for the analysis and proposed
    fix and Sergei for the testcase.
    
            PR rtl-optimization/115876
            PR rtl-optimization/115916
    gcc/
            * ext-dce.cc (carry_backpropagate): Make return type unsigned as well.
            Cast to signed for right shift to preserve sign bit.
    
    gcc/testsuite/
    
            * g++.dg/torture/pr115916.C: New test.
    
            Co-author: Andreas Schwab <schwab@linux-m68k.org>
            Co-author: Sergei Trofimovich <slyfox at gentoo dot org>

Comments

Andreas Schwab July 17, 2024, 6:46 a.m. UTC | #1 
On Jul 15 2024, Jeff Law wrote:

> My change to fix a ubsan issue broke handling propagation of the
> carry/sign bit down through a right shift.

What about the other ASHIFTs?
Jeff Law July 17, 2024, 2:48 p.m. UTC | #2 
On 7/17/24 12:46 AM, Andreas Schwab wrote:
> On Jul 15 2024, Jeff Law wrote:
> 
>> My change to fix a ubsan issue broke handling propagation of the
>> carry/sign bit down through a right shift.
> 
> What about the other ASHIFTs?
They're on my list.  Just didn't have the time to work through those 
cases mentally and didn't want to leave things in a badly broken state.

jeff
diff mbox series

Patch

diff --git a/gcc/ext-dce.cc b/gcc/ext-dce.cc
index 91789d283fc..2869a389c3a 100644
--- a/gcc/ext-dce.cc
+++ b/gcc/ext-dce.cc
@@ -373,7 +373,7 @@  binop_implies_op2_fully_live (rtx_code code)
    binop_implies_op2_fully_live (e.g. shifts), the computed mask may
    exclusively pertain to the first operand.  */
 
-HOST_WIDE_INT
+unsigned HOST_WIDE_INT
 carry_backpropagate (unsigned HOST_WIDE_INT mask, enum rtx_code code, rtx x)
 {
   if (mask == 0)
@@ -393,7 +393,7 @@  carry_backpropagate (unsigned HOST_WIDE_INT mask, enum rtx_code code, rtx x)
     case ASHIFT:
       if (CONSTANT_P (XEXP (x, 1))
 	  && known_lt (UINTVAL (XEXP (x, 1)), GET_MODE_BITSIZE (mode)))
-	return mask >> INTVAL (XEXP (x, 1));
+	return (HOST_WIDE_INT)mask >> INTVAL (XEXP (x, 1));
       return (2ULL << floor_log2 (mask)) - 1;
 
     /* We propagate for the shifted operand, but not the shift
diff --git a/gcc/testsuite/g++.dg/torture/pr115916.C b/gcc/testsuite/g++.dg/torture/pr115916.C
new file mode 100644
index 00000000000..3d788678eaa
--- /dev/null
+++ b/gcc/testsuite/g++.dg/torture/pr115916.C
@@ -0,0 +1,90 @@ 
+/* { dg-do run } */
+
+#include <stddef.h>
+#include <stdint.h>
+
+struct ve {
+    ve() = default;
+    ve(const ve&) = default;
+    ve& operator=(const ve&) = default;
+
+    // note that the code usually uses the first half of this array
+    uint8_t raw[16] = {};
+};
+
+static ve First8_(void) {
+    ve m;
+    __builtin_memset(m.raw, 0xff, 8);
+    return m;
+}
+
+static ve And_(ve a, ve b) {
+    ve au;
+    __builtin_memcpy(au.raw, a.raw, 16);
+    for (size_t i = 0; i < 8; ++i) {
+        au.raw[i] &= b.raw[i];
+    }
+    return au;
+}
+
+__attribute__((noipa, optimize(0)))
+static void vec_assert(ve a) {
+    if (a.raw[6] != 0x06 && a.raw[6] != 0x07)
+        __builtin_trap();
+}
+
+static ve Reverse4_(ve v) {
+    ve ret;
+    for (size_t i = 0; i < 8; i += 4) {
+        ret.raw[i + 0] = v.raw[i + 3];
+        ret.raw[i + 1] = v.raw[i + 2];
+        ret.raw[i + 2] = v.raw[i + 1];
+        ret.raw[i + 3] = v.raw[i + 0];
+    }
+    return ret;
+}
+
+static ve DupEven_(ve v) {
+    for (size_t i = 0; i < 8; i += 2) {
+        v.raw[i + 1] = v.raw[i];
+    }
+    return v;
+}
+
+template <bool b>
+ve Per4LaneBlockShuffle_(ve v) {
+    if (b) {
+        return Reverse4_(v);
+    } else {
+        return DupEven_(v);
+    }
+}
+
+template <bool b>
+static inline __attribute__((always_inline)) void DoTestPer4LaneBlkShuffle(const ve v) {
+    ve actual = Per4LaneBlockShuffle_<b>(v);
+    const auto valid_lanes_mask = First8_();
+    ve actual_masked = And_(valid_lanes_mask, actual);
+    vec_assert(actual_masked);
+}
+
+static void DoTestPer4LaneBlkShuffles(const ve v) {
+    alignas(128) uint8_t src_lanes[8];
+    __builtin_memcpy(src_lanes, v.raw, 8);
+    // need both, hm
+    DoTestPer4LaneBlkShuffle<true >(v);
+    DoTestPer4LaneBlkShuffle<false>(v);
+}
+
+__attribute__((noipa, optimize(0)))
+static void bug(void) {
+   uint8_t iv[8] = {1,2,3,4,5,6,7,8};
+   ve v;
+   __builtin_memcpy(v.raw, iv, 8);
+   DoTestPer4LaneBlkShuffles(v);
+}
+
+int main(void) {
+    bug();
+}
+