Message ID | patch-16562-tamar@arm.com |
---|---|
State | Committed |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 88F9D3858D35 for <patchwork@sourceware.org>; Fri, 11 Nov 2022 14:46:26 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 88F9D3858D35 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1668177986; bh=g24/hkA1owqtP1O4wNxe573+9lOHOAbLz4ySClXN9DI=; h=Date:To:Cc:Subject:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:List-Subscribe:From:Reply-To:From; b=qyCZbxZEJpIG/e4hikgsxCqSrwIwwKHUJ2SVt9AA58fptgWPIWnDUVnD05pXO+qW2 2TabovMbAb6o1zdpTpmkFeWItemNc7FcS66035rm+YOKsY+Lg936oe8LnfezQQ/S2x Px/xLvo3TP00cKHuFQsSxLl67UEWmlOpyJYJZhvs= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from EUR01-DB5-obe.outbound.protection.outlook.com (mail-eopbgr150050.outbound.protection.outlook.com [40.107.15.50]) by sourceware.org (Postfix) with ESMTPS id B08503858D1E for <gcc-patches@gcc.gnu.org>; Fri, 11 Nov 2022 14:45:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B08503858D1E ARC-Seal: i=2; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=pass; b=EczzsLinER+5pc1oh03lsx886BChdWx9toUa9eYjuv8SuDgBsQNedYNNh38rID2op2yFgFbMRk0USMTH8G5jk0oeKJQzE0fe1prfG9jrLF5yvGMaOOGfTtGCj3lT7XtRMHO7XnBv5csRgr6pz/Oer5JTZwBx/vSw+UrguRLiVB2YNe8+Sll5BUwn84f6B2w/Yr76YzsuPmmmGkot+rvf/b1kbDmOZ1RDnny2auPa6qEvScjRLxC8gDt2dR8C9/sXbHHGhk5XaAP7oHHfV9SVvySMhzytPFVBuMYVyFo0E5hqIryaHIjM+cD1szaKQ3NG18y448r8LNFC/VdE+jOyRg== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=g24/hkA1owqtP1O4wNxe573+9lOHOAbLz4ySClXN9DI=; b=iVnC0yrUMvpqZx2pbfB1xHz1a4eOLxkFqWhKWxUzT4whaUn96Run7KMTqBmSftC7v+fnCbJ9JroCm7DXbg0972c2cQVzyou2IqDWbnm5kE1tHqsihPGw+kosCwMOCperfj3rH+GxmXkbHOv15YAgjP50BEbsAHMr0G2DzpwEZrXJBtvw59HZ/xW6/KcCGRpTm8zS1iSgeilhpEaOOHnxifDHbasHbqXfARP1XY85nijZZ2fJj7bCky6Apvo8hHz3Ib3ms4yQebjeBDsFo8rhV3MAVbdg89NceJ8VaKtqQ2UI4A4x7Td+TYjxK+LsNmdf/GK9qwGtteWKaWMKa1/gCg== ARC-Authentication-Results: i=2; mx.microsoft.com 1; spf=pass (sender ip is 63.35.35.123) smtp.rcpttodomain=gcc.gnu.org smtp.mailfrom=arm.com; dmarc=pass (p=none sp=none pct=100) action=none header.from=arm.com; dkim=pass (signature was verified) header.d=armh.onmicrosoft.com; arc=pass (0 oda=1 ltdi=1 spf=[1,1,smtp.mailfrom=arm.com] dkim=[1,1,header.d=arm.com] dmarc=[1,1,header.from=arm.com]) Received: from AS8PR04CA0120.eurprd04.prod.outlook.com (2603:10a6:20b:31e::35) by AS4PR08MB8072.eurprd08.prod.outlook.com (2603:10a6:20b:58b::7) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5813.13; Fri, 11 Nov 2022 14:45:53 +0000 Received: from AM7EUR03FT013.eop-EUR03.prod.protection.outlook.com (2603:10a6:20b:31e:cafe::90) by AS8PR04CA0120.outlook.office365.com (2603:10a6:20b:31e::35) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5813.14 via Frontend Transport; Fri, 11 Nov 2022 14:45:53 +0000 X-MS-Exchange-Authentication-Results: spf=pass (sender IP is 63.35.35.123) smtp.mailfrom=arm.com; dkim=pass (signature was verified) header.d=armh.onmicrosoft.com;dmarc=pass action=none header.from=arm.com; Received-SPF: Pass (protection.outlook.com: domain of arm.com designates 63.35.35.123 as permitted sender) receiver=protection.outlook.com; client-ip=63.35.35.123; helo=64aa7808-outbound-1.mta.getcheckrecipient.com; pr=C Received: from 64aa7808-outbound-1.mta.getcheckrecipient.com (63.35.35.123) by AM7EUR03FT013.mail.protection.outlook.com (100.127.140.191) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5813.12 via Frontend Transport; Fri, 11 Nov 2022 14:45:53 +0000 Received: ("Tessian outbound 6c699027a257:v130"); Fri, 11 Nov 2022 14:45:53 +0000 X-CheckRecipientChecked: true X-CR-MTA-CID: adbe3d255415cc26 X-CR-MTA-TID: 64aa7808 Received: from c8ea31568b78.2 by 64aa7808-outbound-1.mta.getcheckrecipient.com id ED016EBB-E5FD-42D9-BD6E-D90CEF57A76D.1; Fri, 11 Nov 2022 14:45:47 +0000 Received: from EUR05-DB8-obe.outbound.protection.outlook.com by 64aa7808-outbound-1.mta.getcheckrecipient.com with ESMTPS id c8ea31568b78.2 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384); Fri, 11 Nov 2022 14:45:47 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=KqV34YjP2xKb27H2RX6igfhxGj7Cz9iweHu+v/bG+OKzun5BM/Ku6N1n90aV8TfGKQyaMrL9L+5TMYTPqAzxYDLH5tuK0A2vTLIcpyi3Awxl+itr5FjtBkZ3JfY2fwRZUHBE4WKtdreiVAFaEu4Jdl5KRSjXXLrt0tr5e6hMIAyvBCpm4ndQ9NWAAXLKLICvVJT83k+sJedVopXNkamk3XeMCOr4WZmt9RE7lYw7SryMi7N2S/tw/Ey6CUU5Yu2WFBVlmWyDB88CZSY5vSOm+rJ+y56TFy0P03xw+mOmBbPecTDWO1PplzwLPetKU69mlIONoupZhlWPcCCELj3oPw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=g24/hkA1owqtP1O4wNxe573+9lOHOAbLz4ySClXN9DI=; b=RJIpF8Xl7ro1HMCHMUK4xoVF5E1v9ogdTIW1tfj1/hvNyLcP3Yd85x5tj1tJu6eSFuaa+mSuPaybNbQ8uy83oCMc+gRippWgidhm3NbswW0GNXozAdueJ+OqVHbv2tQHwS2GN5sxw5N6JLJLP3k3UzlmMeoRraZj9fuj0evdgaedbVL0gmpv8MYmIwdHxaQpH8sJvxuyeICRjw/gjJcGV3a1TXP+wp1EglNd6qnMru2/whefh6fV3/MT7TTXUv3yc/f5HSSBO7ehsonnTo7KlggleP7zAiI+br3A9O+S1HX8mxUVbjccyQw/jn2r8BRiLv/OqQEGoZBTHkFtUlZ2ug== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none Authentication-Results-Original: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com; Received: from VI1PR08MB5325.eurprd08.prod.outlook.com (2603:10a6:803:13e::17) by GV2PR08MB8344.eurprd08.prod.outlook.com (2603:10a6:150:c0::17) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5813.13; Fri, 11 Nov 2022 14:45:45 +0000 Received: from VI1PR08MB5325.eurprd08.prod.outlook.com ([fe80::bd2a:aff9:b1a0:2fc7]) by VI1PR08MB5325.eurprd08.prod.outlook.com ([fe80::bd2a:aff9:b1a0:2fc7%4]) with mapi id 15.20.5813.013; Fri, 11 Nov 2022 14:45:45 +0000 Date: Fri, 11 Nov 2022 14:45:42 +0000 To: gcc-patches@gcc.gnu.org Cc: nd@arm.com, Richard.Earnshaw@arm.com, Marcus.Shawcroft@arm.com, Kyrylo.Tkachov@arm.com, richard.sandiford@arm.com Subject: [PATCH]AArch64 Fix vector re-interpretation between partial SIMD modes Message-ID: <patch-16562-tamar@arm.com> Content-Type: multipart/mixed; boundary="k9WQWxCtuRWVMnF8" Content-Disposition: inline X-ClientProxiedBy: LO2P265CA0431.GBRP265.PROD.OUTLOOK.COM (2603:10a6:600:a0::35) To VI1PR08MB5325.eurprd08.prod.outlook.com (2603:10a6:803:13e::17) MIME-Version: 1.0 X-MS-TrafficTypeDiagnostic: VI1PR08MB5325:EE_|GV2PR08MB8344:EE_|AM7EUR03FT013:EE_|AS4PR08MB8072:EE_ X-MS-Office365-Filtering-Correlation-Id: f28168ab-79f8-45eb-ad4a-08dac3f36f43 x-checkrecipientrouted: true NoDisclaimer: true X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam-Untrusted: BCL:0; X-Microsoft-Antispam-Message-Info-Original: Ibb7FIEPQe4OOVRkr+Lv97O3zDmBLjhblAeTXj6Ic9LLQvU7n5ZadZpOUxGLkf6JcH/xp1wBshUuXbkEJAoHbKhC9K+HOFuRozl6BoAb88o5ps8Mms5uoWjRau0+AmraxYFP0++wen1RWfPiVYfglQauG8kLp7KeR9Ev2CU9jt0phPKBCWfn7TeWdObBPc5KcHzVQ+eoBAq6L4fp28s19R5VgzkmN73LLy3Ev/HwPyY3Chajrt0xQ+FmSsK5vL9J9DcSQcfoZgUFpkzU4EwergkcSiq9PmXX4c8Qf0NOOJOtAduLM4j4qh685SuwZEJNQ7wptggo6HXxbwu/qpSB8HicgrJUwrxNLgbZx8D5c4v8wzceSKYQFMJFCPvui8XYgn0qMtsKS5R3e3/5oZGWzTTzn67zgVzHUUhw308Rq6YaN9PUeGxH66fk5xhC7YnRxtelP+g52FVBVnbmFDe6vVucOfaKi1wUMvK3dRBr1hhSdaJ1pzfS4J9ImAYviYd1VVUPuNwgM/rFnOi5K9DRHB0hA6QJiCWORNihkgCOMhSncoiqxhtWMDe688FSVBGNdzWMpDJoDJICZGh0dRh8ysb/UXfq2k4wb7YYMqXly99xEAfjBK5zJ3sL5obmUdDCGh0hOAnuPGrT7/mCXu8zljDrYipafHett6X8e8WdjvwWmUdAm6oXAj2J2i3KodHaVhR8XtPbyo/gPe4GnWABM1sVJx15SUECuTshATIEnpAnlB1oXaPHYaGALFHa2Da/BGScdlhCRlo1lDNuYSou5w== X-Forefront-Antispam-Report-Untrusted: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:VI1PR08MB5325.eurprd08.prod.outlook.com; PTR:; CAT:NONE; SFS:(13230022)(4636009)(376002)(136003)(396003)(346002)(366004)(39860400002)(451199015)(36756003)(66476007)(6512007)(2906002)(66946007)(8676002)(4326008)(66556008)(8936002)(6506007)(33964004)(44832011)(44144004)(41300700001)(6666004)(5660300002)(235185007)(478600001)(4743002)(6486002)(26005)(2616005)(186003)(86362001)(38100700002)(6916009)(316002)(4216001)(2700100001); DIR:OUT; SFP:1101; X-MS-Exchange-Transport-CrossTenantHeadersStamped: GV2PR08MB8344 Original-Authentication-Results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com; X-EOPAttributedMessage: 0 X-MS-Exchange-Transport-CrossTenantHeadersStripped: AM7EUR03FT013.eop-EUR03.prod.protection.outlook.com X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id-Prvs: d91fc469-0b80-4ed1-8bc4-08dac3f36a39 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: MT/M/cQp5orRm+Ux9KTwnLKoppP8celg+buV+MhF6FxmXTBBAhuflWdsD8zei1gcW09y+eSY7KxD+tBSAHyCvZ3U6MKF7SbMRS/ei9BQZehnckg+2OTwkqD1qRP98BVojEIp3CYNI4x1KRv5WJ5uO9u5wyxGmN2hon8aYfAwctw38hyJf2xb6iPCM48ZoexmQ763ngHUQh+6TlQQJYTD4fqRfd7fei6GgfUc0URHs1VuuvxaMwt9Wk/grdKxtOJvb7a+UA2jdYzaqHfuJKumt9zFrZFOgBZZRVXvphwlJ2prUtYm7G3XMPmTX8Up3xPIIh/2Iyl7tOhP/p3Rp/HvVE2BY4JbnX9p5sHldb9I4xvSysIGiJI3QfYgF53giPTpHjjCpetJLSuH8ReVWDCwx79ettEfP4tBO/oZEIaSZC+iaw+OGrIhiEoJigCz3xqLGJ56Urx0HmwmtEytF5UwrdWY+ShB81aRdA+p+PcsJfbvFrGzMVWWTDrRGHUE/+yq4IQ6xHMn5165BSauMojsmD+jvYZQn8bNQVVTydcx9kHJ2rtFz6/XN1/P2AFNnQH/eOB+TQcwoA7zlWx+QXxSEulllwxvcsqG4hXa4Odeoy2KMM1w4AJcOfnGXFUpj0xFLYejOhIIJFBYTFQLtMIwwCV+XymZYuXyp4YbXSDBtC5G7vlSqtHS7X++7OrwiQXv1Tz9cmgtnAQTYszIgPs1a2FnIDznWXuiGwQWAcEqtGcdzCO8aLdhKhtAjApJ7pKmXjgr5Xq8UIQjXiS4UGEjGV5SKeenjrSCSmr656JIYC1aPcis/5wOyT7VbJ+sC4gv X-Forefront-Antispam-Report: CIP:63.35.35.123; CTRY:IE; LANG:en; SCL:1; SRV:; IPV:CAL; SFV:NSPM; H:64aa7808-outbound-1.mta.getcheckrecipient.com; PTR:ec2-63-35-35-123.eu-west-1.compute.amazonaws.com; CAT:NONE; SFS:(13230022)(4636009)(136003)(376002)(396003)(346002)(39860400002)(451199015)(46966006)(36840700001)(40470700004)(70206006)(44832011)(82310400005)(82740400003)(2906002)(86362001)(356005)(40460700003)(6666004)(6916009)(4326008)(6506007)(81166007)(26005)(6512007)(44144004)(40480700001)(33964004)(4743002)(2616005)(316002)(478600001)(70586007)(8676002)(6486002)(36860700001)(186003)(5660300002)(336012)(8936002)(41300700001)(235185007)(36756003)(47076005)(4216001)(2700100001); DIR:OUT; SFP:1101; X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Nov 2022 14:45:53.6584 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: f28168ab-79f8-45eb-ad4a-08dac3f36f43 X-MS-Exchange-CrossTenant-Id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=f34e5979-57d9-4aaa-ad4d-b122a662184d; Ip=[63.35.35.123]; Helo=[64aa7808-outbound-1.mta.getcheckrecipient.com] X-MS-Exchange-CrossTenant-AuthSource: AM7EUR03FT013.eop-EUR03.prod.protection.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Anonymous X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS4PR08MB8072 X-Spam-Status: No, score=-12.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, FORGED_SPF_HELO, GIT_PATCH_0, KAM_DMARC_NONE, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_PASS, SPF_NONE, TXREP, UNPARSEABLE_RELAY autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Tamar Christina via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Tamar Christina <tamar.christina@arm.com> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
AArch64 Fix vector re-interpretation between partial SIMD modes
|
|
Commit Message
Tamar Christina
Nov. 11, 2022, 2:45 p.m. UTC
Hi All, While writing a patch series I started getting incorrect codegen out from VEC_PERM on partial struct types. It turns out that this was happening because the TARGET_CAN_CHANGE_MODE_CLASS implementation has a slight bug in it. The hook only checked for SIMD to Partial but never Partial to SIMD. This resulted in incorrect subregs to be generated from the fallback code in VEC_PERM_EXPR expansions. I have unfortunately not been able to trigger it using a standalone testcase as the mid-end optimizes away the permute every time I try to describe a permute that would result in the bug. The patch now rejects any conversion of partial SIMD struct types, unless they are both partial structures of the same number of registers or one is a SIMD type who's size is less than 8 bytes. Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. Ok for master? And backport to GCC 12? Thanks, Tamar gcc/ChangeLog: * config/aarch64/aarch64.cc (aarch64_can_change_mode_class): Restrict conversions between partial struct types properly. --- inline copy of patch -- diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc index d3c3650d7d728f56adb65154127dc7b72386c5a7..84dbe2f4ea7d03b424602ed98a34e7824217dc91 100644 -- diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc index d3c3650d7d728f56adb65154127dc7b72386c5a7..84dbe2f4ea7d03b424602ed98a34e7824217dc91 100644 --- a/gcc/config/aarch64/aarch64.cc +++ b/gcc/config/aarch64/aarch64.cc @@ -26471,9 +26471,10 @@ aarch64_can_change_mode_class (machine_mode from, bool from_pred_p = (from_flags & VEC_SVE_PRED); bool to_pred_p = (to_flags & VEC_SVE_PRED); - bool from_full_advsimd_struct_p = (from_flags == (VEC_ADVSIMD | VEC_STRUCT)); bool to_partial_advsimd_struct_p = (to_flags == (VEC_ADVSIMD | VEC_STRUCT | VEC_PARTIAL)); + bool from_partial_advsimd_struct_p = (from_flags == (VEC_ADVSIMD | VEC_STRUCT + | VEC_PARTIAL)); /* Don't allow changes between predicate modes and other modes. Only predicate registers can hold predicate modes and only @@ -26496,9 +26497,23 @@ aarch64_can_change_mode_class (machine_mode from, return false; /* Don't allow changes between partial and full Advanced SIMD structure - modes. */ - if (from_full_advsimd_struct_p && to_partial_advsimd_struct_p) - return false; + modes unless both are a partial struct with the same number of registers + or the vector bitsizes must be the same. */ + if (to_partial_advsimd_struct_p ^ from_partial_advsimd_struct_p) + { + /* If they're both partial structures, allow if they have the same number + or registers. */ + if (to_partial_advsimd_struct_p == from_partial_advsimd_struct_p) + return known_eq (GET_MODE_SIZE (from), GET_MODE_SIZE (to)); + + /* If one is a normal SIMD register, allow only if no larger than 64-bit. */ + if ((to_flags & VEC_ADVSIMD) == to_flags) + return known_le (GET_MODE_SIZE (to), 8); + else if ((from_flags & VEC_ADVSIMD) == from_flags) + return known_le (GET_MODE_SIZE (from), 8); + + return false; + } if (maybe_ne (BITS_PER_SVE_VECTOR, 128u)) {
Comments
Tamar Christina <tamar.christina@arm.com> writes: > Hi All, > > While writing a patch series I started getting incorrect codegen out from > VEC_PERM on partial struct types. > > It turns out that this was happening because the TARGET_CAN_CHANGE_MODE_CLASS > implementation has a slight bug in it. The hook only checked for SIMD to > Partial but never Partial to SIMD. This resulted in incorrect subregs to be > generated from the fallback code in VEC_PERM_EXPR expansions. > > I have unfortunately not been able to trigger it using a standalone testcase as > the mid-end optimizes away the permute every time I try to describe a permute > that would result in the bug. > > The patch now rejects any conversion of partial SIMD struct types, unless they > are both partial structures of the same number of registers or one is a SIMD > type who's size is less than 8 bytes. > > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > > Ok for master? And backport to GCC 12? > > Thanks, > Tamar > > gcc/ChangeLog: > > * config/aarch64/aarch64.cc (aarch64_can_change_mode_class): Restrict > conversions between partial struct types properly. > > --- inline copy of patch -- > diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc > index d3c3650d7d728f56adb65154127dc7b72386c5a7..84dbe2f4ea7d03b424602ed98a34e7824217dc91 100644 > --- a/gcc/config/aarch64/aarch64.cc > +++ b/gcc/config/aarch64/aarch64.cc > @@ -26471,9 +26471,10 @@ aarch64_can_change_mode_class (machine_mode from, > bool from_pred_p = (from_flags & VEC_SVE_PRED); > bool to_pred_p = (to_flags & VEC_SVE_PRED); > > - bool from_full_advsimd_struct_p = (from_flags == (VEC_ADVSIMD | VEC_STRUCT)); > bool to_partial_advsimd_struct_p = (to_flags == (VEC_ADVSIMD | VEC_STRUCT > | VEC_PARTIAL)); > + bool from_partial_advsimd_struct_p = (from_flags == (VEC_ADVSIMD | VEC_STRUCT > + | VEC_PARTIAL)); > > /* Don't allow changes between predicate modes and other modes. > Only predicate registers can hold predicate modes and only > @@ -26496,9 +26497,23 @@ aarch64_can_change_mode_class (machine_mode from, > return false; > > /* Don't allow changes between partial and full Advanced SIMD structure > - modes. */ > - if (from_full_advsimd_struct_p && to_partial_advsimd_struct_p) > - return false; > + modes unless both are a partial struct with the same number of registers > + or the vector bitsizes must be the same. */ > + if (to_partial_advsimd_struct_p ^ from_partial_advsimd_struct_p) > + { > + /* If they're both partial structures, allow if they have the same number > + or registers. */ > + if (to_partial_advsimd_struct_p == from_partial_advsimd_struct_p) > + return known_eq (GET_MODE_SIZE (from), GET_MODE_SIZE (to)); It looks like the ^ makes this line unreachable. I guess it should be a separate top-level condition. > + /* If one is a normal SIMD register, allow only if no larger than 64-bit. */ > + if ((to_flags & VEC_ADVSIMD) == to_flags) > + return known_le (GET_MODE_SIZE (to), 8); > + else if ((from_flags & VEC_ADVSIMD) == from_flags) > + return known_le (GET_MODE_SIZE (from), 8); > + > + return false; > + } I don't think we need to restrict this to SIMD modes. A plain DI would be OK too. So I think it should just be: return (known_le (GET_MODE_SIZE (to), 8) || known_le (GET_MODE_SIZE (from, 8)); Thanks, Richard > > if (maybe_ne (BITS_PER_SVE_VECTOR, 128u)) > {
Richard Sandiford via Gcc-patches <gcc-patches@gcc.gnu.org> writes: > Tamar Christina <tamar.christina@arm.com> writes: >> Hi All, >> >> While writing a patch series I started getting incorrect codegen out from >> VEC_PERM on partial struct types. >> >> It turns out that this was happening because the TARGET_CAN_CHANGE_MODE_CLASS >> implementation has a slight bug in it. The hook only checked for SIMD to >> Partial but never Partial to SIMD. This resulted in incorrect subregs to be >> generated from the fallback code in VEC_PERM_EXPR expansions. >> >> I have unfortunately not been able to trigger it using a standalone testcase as >> the mid-end optimizes away the permute every time I try to describe a permute >> that would result in the bug. >> >> The patch now rejects any conversion of partial SIMD struct types, unless they >> are both partial structures of the same number of registers or one is a SIMD >> type who's size is less than 8 bytes. >> >> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. >> >> Ok for master? And backport to GCC 12? >> >> Thanks, >> Tamar >> >> gcc/ChangeLog: >> >> * config/aarch64/aarch64.cc (aarch64_can_change_mode_class): Restrict >> conversions between partial struct types properly. >> >> --- inline copy of patch -- >> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc >> index d3c3650d7d728f56adb65154127dc7b72386c5a7..84dbe2f4ea7d03b424602ed98a34e7824217dc91 100644 >> --- a/gcc/config/aarch64/aarch64.cc >> +++ b/gcc/config/aarch64/aarch64.cc >> @@ -26471,9 +26471,10 @@ aarch64_can_change_mode_class (machine_mode from, >> bool from_pred_p = (from_flags & VEC_SVE_PRED); >> bool to_pred_p = (to_flags & VEC_SVE_PRED); >> >> - bool from_full_advsimd_struct_p = (from_flags == (VEC_ADVSIMD | VEC_STRUCT)); >> bool to_partial_advsimd_struct_p = (to_flags == (VEC_ADVSIMD | VEC_STRUCT >> | VEC_PARTIAL)); >> + bool from_partial_advsimd_struct_p = (from_flags == (VEC_ADVSIMD | VEC_STRUCT >> + | VEC_PARTIAL)); >> >> /* Don't allow changes between predicate modes and other modes. >> Only predicate registers can hold predicate modes and only >> @@ -26496,9 +26497,23 @@ aarch64_can_change_mode_class (machine_mode from, >> return false; >> >> /* Don't allow changes between partial and full Advanced SIMD structure >> - modes. */ >> - if (from_full_advsimd_struct_p && to_partial_advsimd_struct_p) >> - return false; >> + modes unless both are a partial struct with the same number of registers >> + or the vector bitsizes must be the same. */ >> + if (to_partial_advsimd_struct_p ^ from_partial_advsimd_struct_p) >> + { >> + /* If they're both partial structures, allow if they have the same number >> + or registers. */ >> + if (to_partial_advsimd_struct_p == from_partial_advsimd_struct_p) >> + return known_eq (GET_MODE_SIZE (from), GET_MODE_SIZE (to)); > > It looks like the ^ makes this line unreachable. I guess it should > be a separate top-level condition. > >> + /* If one is a normal SIMD register, allow only if no larger than 64-bit. */ >> + if ((to_flags & VEC_ADVSIMD) == to_flags) >> + return known_le (GET_MODE_SIZE (to), 8); >> + else if ((from_flags & VEC_ADVSIMD) == from_flags) >> + return known_le (GET_MODE_SIZE (from), 8); >> + >> + return false; >> + } > > I don't think we need to restrict this to SIMD modes. A plain DI would > be OK too. So I think it should just be: > > return (known_le (GET_MODE_SIZE (to), 8) > || known_le (GET_MODE_SIZE (from, 8)); Looking again, all the other tests return false if they found a definite problem and fall through to later code otherwise. I think we should do the same here. Thanks, Richard
> -----Original Message----- > From: Richard Sandiford <richard.sandiford@arm.com> > Sent: Friday, November 18, 2022 9:30 AM > To: Tamar Christina <Tamar.Christina@arm.com> > Cc: gcc-patches@gcc.gnu.org; nd <nd@arm.com>; Richard Earnshaw > <Richard.Earnshaw@arm.com>; Marcus Shawcroft > <Marcus.Shawcroft@arm.com>; Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> > Subject: Re: [PATCH]AArch64 Fix vector re-interpretation between partial > SIMD modes > > Richard Sandiford via Gcc-patches <gcc-patches@gcc.gnu.org> writes: > > Tamar Christina <tamar.christina@arm.com> writes: > >> Hi All, > >> > >> While writing a patch series I started getting incorrect codegen out > >> from VEC_PERM on partial struct types. > >> > >> It turns out that this was happening because the > >> TARGET_CAN_CHANGE_MODE_CLASS implementation has a slight bug in > it. The hook only checked for SIMD to > >> Partial but never Partial to SIMD. This resulted in incorrect subregs to be > >> generated from the fallback code in VEC_PERM_EXPR expansions. > >> > >> I have unfortunately not been able to trigger it using a standalone > >> testcase as the mid-end optimizes away the permute every time I try > >> to describe a permute that would result in the bug. > >> > >> The patch now rejects any conversion of partial SIMD struct types, > >> unless they are both partial structures of the same number of > >> registers or one is a SIMD type who's size is less than 8 bytes. > >> > >> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > >> > >> Ok for master? And backport to GCC 12? > >> > >> Thanks, > >> Tamar > >> > >> gcc/ChangeLog: > >> > >> * config/aarch64/aarch64.cc (aarch64_can_change_mode_class): > Restrict > >> conversions between partial struct types properly. > >> > >> --- inline copy of patch -- > >> diff --git a/gcc/config/aarch64/aarch64.cc > >> b/gcc/config/aarch64/aarch64.cc index > >> > d3c3650d7d728f56adb65154127dc7b72386c5a7..84dbe2f4ea7d03b424602ed9 > 8a3 > >> 4e7824217dc91 100644 > >> --- a/gcc/config/aarch64/aarch64.cc > >> +++ b/gcc/config/aarch64/aarch64.cc > >> @@ -26471,9 +26471,10 @@ aarch64_can_change_mode_class > (machine_mode from, > >> bool from_pred_p = (from_flags & VEC_SVE_PRED); > >> bool to_pred_p = (to_flags & VEC_SVE_PRED); > >> > >> - bool from_full_advsimd_struct_p = (from_flags == (VEC_ADVSIMD | > VEC_STRUCT)); > >> bool to_partial_advsimd_struct_p = (to_flags == (VEC_ADVSIMD | > VEC_STRUCT > >> | VEC_PARTIAL)); > >> + bool from_partial_advsimd_struct_p = (from_flags == (VEC_ADVSIMD > | VEC_STRUCT > >> + | VEC_PARTIAL)); > >> > >> /* Don't allow changes between predicate modes and other modes. > >> Only predicate registers can hold predicate modes and only @@ > >> -26496,9 +26497,23 @@ aarch64_can_change_mode_class > (machine_mode from, > >> return false; > >> > >> /* Don't allow changes between partial and full Advanced SIMD > structure > >> - modes. */ > >> - if (from_full_advsimd_struct_p && to_partial_advsimd_struct_p) > >> - return false; > >> + modes unless both are a partial struct with the same number of > registers > >> + or the vector bitsizes must be the same. */ > >> + if (to_partial_advsimd_struct_p ^ from_partial_advsimd_struct_p) > >> + { > >> + /* If they're both partial structures, allow if they have the same > number > >> + or registers. */ > >> + if (to_partial_advsimd_struct_p == from_partial_advsimd_struct_p) > >> + return known_eq (GET_MODE_SIZE (from), GET_MODE_SIZE (to)); > > > > It looks like the ^ makes this line unreachable. I guess it should be > > a separate top-level condition. > > > >> + /* If one is a normal SIMD register, allow only if no larger than 64-bit. > */ > >> + if ((to_flags & VEC_ADVSIMD) == to_flags) > >> + return known_le (GET_MODE_SIZE (to), 8); > >> + else if ((from_flags & VEC_ADVSIMD) == from_flags) > >> + return known_le (GET_MODE_SIZE (from), 8); > >> + > >> + return false; > >> + } > > > > I don't think we need to restrict this to SIMD modes. A plain DI > > would be OK too. So I think it should just be: > > > > return (known_le (GET_MODE_SIZE (to), 8) > > || known_le (GET_MODE_SIZE (from, 8)); > > Looking again, all the other tests return false if they found a definite problem > and fall through to later code otherwise. I think we should do the same here. I've rewritten the conditions. I needed to allow any conversions as long as they're both partial vectors. There were various intrinsics tests that rely on for instance being able to take a subreg of a VN2x8QI from a VN3x8QI for instance. I did not only allow the smaller case since it didn't seem logical to block paradoxical subregs of this kind. Reload seems to be correctly handling them as separate 64-bit registers (we have tests for this it looks like.) So Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. Ok for master? And backport to GCC 12? Thanks, Tamar gcc/ChangeLog: * config/aarch64/aarch64.cc (aarch64_can_change_mode_class): Restrict conversions between partial struct types properly. ---- inline copy of patch --- diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc index c91df6f5006c257690aafb75398933d628a970e1..ff02c78f895b26a70b2653e58db453f4b5870666 100644 --- a/gcc/config/aarch64/aarch64.cc +++ b/gcc/config/aarch64/aarch64.cc @@ -26658,9 +26658,10 @@ aarch64_can_change_mode_class (machine_mode from, bool from_pred_p = (from_flags & VEC_SVE_PRED); bool to_pred_p = (to_flags & VEC_SVE_PRED); - bool from_full_advsimd_struct_p = (from_flags == (VEC_ADVSIMD | VEC_STRUCT)); bool to_partial_advsimd_struct_p = (to_flags == (VEC_ADVSIMD | VEC_STRUCT | VEC_PARTIAL)); + bool from_partial_advsimd_struct_p = (from_flags == (VEC_ADVSIMD | VEC_STRUCT + | VEC_PARTIAL)); /* Don't allow changes between predicate modes and other modes. Only predicate registers can hold predicate modes and only @@ -26682,9 +26683,10 @@ aarch64_can_change_mode_class (machine_mode from, || GET_MODE_UNIT_SIZE (from) != GET_MODE_UNIT_SIZE (to))) return false; - /* Don't allow changes between partial and full Advanced SIMD structure - modes. */ - if (from_full_advsimd_struct_p && to_partial_advsimd_struct_p) + /* Don't allow changes between partial and other registers only if + one is a normal SIMD register, allow only if not larger than 64-bit. */ + if ((to_partial_advsimd_struct_p ^ from_partial_advsimd_struct_p) + && (known_gt (GET_MODE_SIZE (to), 8) || known_gt (GET_MODE_SIZE (to), 8))) return false; if (maybe_ne (BITS_PER_SVE_VECTOR, 128u))
Tamar Christina <Tamar.Christina@arm.com> writes: >> -----Original Message----- >> From: Richard Sandiford <richard.sandiford@arm.com> >> Sent: Friday, November 18, 2022 9:30 AM >> To: Tamar Christina <Tamar.Christina@arm.com> >> Cc: gcc-patches@gcc.gnu.org; nd <nd@arm.com>; Richard Earnshaw >> <Richard.Earnshaw@arm.com>; Marcus Shawcroft >> <Marcus.Shawcroft@arm.com>; Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> >> Subject: Re: [PATCH]AArch64 Fix vector re-interpretation between partial >> SIMD modes >> >> Richard Sandiford via Gcc-patches <gcc-patches@gcc.gnu.org> writes: >> > Tamar Christina <tamar.christina@arm.com> writes: >> >> Hi All, >> >> >> >> While writing a patch series I started getting incorrect codegen out >> >> from VEC_PERM on partial struct types. >> >> >> >> It turns out that this was happening because the >> >> TARGET_CAN_CHANGE_MODE_CLASS implementation has a slight bug in >> it. The hook only checked for SIMD to >> >> Partial but never Partial to SIMD. This resulted in incorrect subregs to be >> >> generated from the fallback code in VEC_PERM_EXPR expansions. >> >> >> >> I have unfortunately not been able to trigger it using a standalone >> >> testcase as the mid-end optimizes away the permute every time I try >> >> to describe a permute that would result in the bug. >> >> >> >> The patch now rejects any conversion of partial SIMD struct types, >> >> unless they are both partial structures of the same number of >> >> registers or one is a SIMD type who's size is less than 8 bytes. >> >> >> >> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. >> >> >> >> Ok for master? And backport to GCC 12? >> >> >> >> Thanks, >> >> Tamar >> >> >> >> gcc/ChangeLog: >> >> >> >> * config/aarch64/aarch64.cc (aarch64_can_change_mode_class): >> Restrict >> >> conversions between partial struct types properly. >> >> >> >> --- inline copy of patch -- >> >> diff --git a/gcc/config/aarch64/aarch64.cc >> >> b/gcc/config/aarch64/aarch64.cc index >> >> >> d3c3650d7d728f56adb65154127dc7b72386c5a7..84dbe2f4ea7d03b424602ed9 >> 8a3 >> >> 4e7824217dc91 100644 >> >> --- a/gcc/config/aarch64/aarch64.cc >> >> +++ b/gcc/config/aarch64/aarch64.cc >> >> @@ -26471,9 +26471,10 @@ aarch64_can_change_mode_class >> (machine_mode from, >> >> bool from_pred_p = (from_flags & VEC_SVE_PRED); >> >> bool to_pred_p = (to_flags & VEC_SVE_PRED); >> >> >> >> - bool from_full_advsimd_struct_p = (from_flags == (VEC_ADVSIMD | >> VEC_STRUCT)); >> >> bool to_partial_advsimd_struct_p = (to_flags == (VEC_ADVSIMD | >> VEC_STRUCT >> >> | VEC_PARTIAL)); >> >> + bool from_partial_advsimd_struct_p = (from_flags == (VEC_ADVSIMD >> | VEC_STRUCT >> >> + | VEC_PARTIAL)); >> >> >> >> /* Don't allow changes between predicate modes and other modes. >> >> Only predicate registers can hold predicate modes and only @@ >> >> -26496,9 +26497,23 @@ aarch64_can_change_mode_class >> (machine_mode from, >> >> return false; >> >> >> >> /* Don't allow changes between partial and full Advanced SIMD >> structure >> >> - modes. */ >> >> - if (from_full_advsimd_struct_p && to_partial_advsimd_struct_p) >> >> - return false; >> >> + modes unless both are a partial struct with the same number of >> registers >> >> + or the vector bitsizes must be the same. */ >> >> + if (to_partial_advsimd_struct_p ^ from_partial_advsimd_struct_p) >> >> + { >> >> + /* If they're both partial structures, allow if they have the same >> number >> >> + or registers. */ >> >> + if (to_partial_advsimd_struct_p == from_partial_advsimd_struct_p) >> >> + return known_eq (GET_MODE_SIZE (from), GET_MODE_SIZE (to)); >> > >> > It looks like the ^ makes this line unreachable. I guess it should be >> > a separate top-level condition. >> > >> >> + /* If one is a normal SIMD register, allow only if no larger than 64-bit. >> */ >> >> + if ((to_flags & VEC_ADVSIMD) == to_flags) >> >> + return known_le (GET_MODE_SIZE (to), 8); >> >> + else if ((from_flags & VEC_ADVSIMD) == from_flags) >> >> + return known_le (GET_MODE_SIZE (from), 8); >> >> + >> >> + return false; >> >> + } >> > >> > I don't think we need to restrict this to SIMD modes. A plain DI >> > would be OK too. So I think it should just be: >> > >> > return (known_le (GET_MODE_SIZE (to), 8) >> > || known_le (GET_MODE_SIZE (from, 8)); >> >> Looking again, all the other tests return false if they found a definite problem >> and fall through to later code otherwise. I think we should do the same here. > > I've rewritten the conditions. I needed to allow any conversions as long as they're both > partial vectors. There were various intrinsics tests that rely on for instance being able to > take a subreg of a VN2x8QI from a VN3x8QI for instance. I did not only allow the smaller > case since it didn't seem logical to block paradoxical subregs of this kind. > > Reload seems to be correctly handling them as separate 64-bit registers (we have tests for > this it looks like.) > > So Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > > Ok for master? And backport to GCC 12? OK for both, thanks. Richard > Thanks, > Tamar > > gcc/ChangeLog: > > * config/aarch64/aarch64.cc (aarch64_can_change_mode_class): Restrict > conversions between partial struct types properly. > > ---- inline copy of patch --- > > diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc > index c91df6f5006c257690aafb75398933d628a970e1..ff02c78f895b26a70b2653e58db453f4b5870666 100644 > --- a/gcc/config/aarch64/aarch64.cc > +++ b/gcc/config/aarch64/aarch64.cc > @@ -26658,9 +26658,10 @@ aarch64_can_change_mode_class (machine_mode from, > bool from_pred_p = (from_flags & VEC_SVE_PRED); > bool to_pred_p = (to_flags & VEC_SVE_PRED); > > - bool from_full_advsimd_struct_p = (from_flags == (VEC_ADVSIMD | VEC_STRUCT)); > bool to_partial_advsimd_struct_p = (to_flags == (VEC_ADVSIMD | VEC_STRUCT > | VEC_PARTIAL)); > + bool from_partial_advsimd_struct_p = (from_flags == (VEC_ADVSIMD | VEC_STRUCT > + | VEC_PARTIAL)); > > /* Don't allow changes between predicate modes and other modes. > Only predicate registers can hold predicate modes and only > @@ -26682,9 +26683,10 @@ aarch64_can_change_mode_class (machine_mode from, > || GET_MODE_UNIT_SIZE (from) != GET_MODE_UNIT_SIZE (to))) > return false; > > - /* Don't allow changes between partial and full Advanced SIMD structure > - modes. */ > - if (from_full_advsimd_struct_p && to_partial_advsimd_struct_p) > + /* Don't allow changes between partial and other registers only if > + one is a normal SIMD register, allow only if not larger than 64-bit. */ > + if ((to_partial_advsimd_struct_p ^ from_partial_advsimd_struct_p) > + && (known_gt (GET_MODE_SIZE (to), 8) || known_gt (GET_MODE_SIZE (to), 8))) > return false; > > if (maybe_ne (BITS_PER_SVE_VECTOR, 128u))
--- a/gcc/config/aarch64/aarch64.cc +++ b/gcc/config/aarch64/aarch64.cc @@ -26471,9 +26471,10 @@ aarch64_can_change_mode_class (machine_mode from, bool from_pred_p = (from_flags & VEC_SVE_PRED); bool to_pred_p = (to_flags & VEC_SVE_PRED); - bool from_full_advsimd_struct_p = (from_flags == (VEC_ADVSIMD | VEC_STRUCT)); bool to_partial_advsimd_struct_p = (to_flags == (VEC_ADVSIMD | VEC_STRUCT | VEC_PARTIAL)); + bool from_partial_advsimd_struct_p = (from_flags == (VEC_ADVSIMD | VEC_STRUCT + | VEC_PARTIAL)); /* Don't allow changes between predicate modes and other modes. Only predicate registers can hold predicate modes and only @@ -26496,9 +26497,23 @@ aarch64_can_change_mode_class (machine_mode from, return false; /* Don't allow changes between partial and full Advanced SIMD structure - modes. */ - if (from_full_advsimd_struct_p && to_partial_advsimd_struct_p) - return false; + modes unless both are a partial struct with the same number of registers + or the vector bitsizes must be the same. */ + if (to_partial_advsimd_struct_p ^ from_partial_advsimd_struct_p) + { + /* If they're both partial structures, allow if they have the same number + or registers. */ + if (to_partial_advsimd_struct_p == from_partial_advsimd_struct_p) + return known_eq (GET_MODE_SIZE (from), GET_MODE_SIZE (to)); + + /* If one is a normal SIMD register, allow only if no larger than 64-bit. */ + if ((to_flags & VEC_ADVSIMD) == to_flags) + return known_le (GET_MODE_SIZE (to), 8); + else if ((from_flags & VEC_ADVSIMD) == from_flags) + return known_le (GET_MODE_SIZE (from), 8); + + return false; + } if (maybe_ne (BITS_PER_SVE_VECTOR, 128u)) {