From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR05-VI1-obe.outbound.protection.outlook.com (mail-vi1eur05olkn2047.outbound.protection.outlook.com [40.92.90.47]) by sourceware.org (Postfix) with ESMTPS id 77A6F3858D29 for ; Mon, 17 Jun 2024 10:41:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 77A6F3858D29 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=hotmail.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=hotmail.de ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 77A6F3858D29 Authentication-Results: server2.sourceware.org; arc=pass smtp.remote-ip=40.92.90.47 ARC-Seal: i=2; a=rsa-sha256; d=sourceware.org; s=key; t=1718620884; cv=pass; b=ZZGgsyPskyg+rDKAdYPC3U4hgBsTUI4+SVdaakSOhwJzw/I++/T0QeHO5MEppemahNBtgckpgrEDqSDQYJb7qWS2UBkXok7iJdLiJGMRQheUqSN6Q0IiGUVaKm1g9XSO9Pnk+VLOM5cBm/6RpQXo6jGxbyC44wHgbjWhi7Ag5n4= ARC-Message-Signature: i=2; a=rsa-sha256; d=sourceware.org; s=key; t=1718620884; c=relaxed/simple; bh=HNiR9MUmVN0YkNk+hbahA2CoxoU0OOvP3lEFHLTWl7w=; h=DKIM-Signature:Message-ID:Date:Subject:To:From:MIME-Version; b=B4ATGhrjdFLyogQTx5dWOXEITB148RTW0rj3/iI68J7lpUTyfl7uoajp8hV/1SnmSe9OzolL0eOgjaPq8zFvJyg5yGLpP/jccjKpLq6jfnvY/JRAz++DIWygJtEWgyLJI5PwhuVG6cn3zUBlzOCM/S3ZX7vFP4eWzo5R7rFtgaY= ARC-Authentication-Results: i=2; server2.sourceware.org ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=cJYgGcUNHjQwyFyyIg+w4bRnAybNu7N0z0XRQBGZQhhYL1PBxF12fhCqPAspZJ/5LH0EnK4M8ZC7HHAuh4+kdn+hzg/mmIqb6TXWxnBzXseAfi2Jk0yK9wY0aGnGugfljsNNDd5v2RBGJLnqN3v6PBro0claVgJRIbGE4oYN53DiBKXcN0NwAwjI1WwkwZJLHJ1CpuwZzgvMMpKJ5vQij0k5L4J91BEtMheOV7FIevTJBOcqwjrWTZXCAw6hR/1IIQ3Th0cl/LcTDBUmi9pxL4Jegk6zgBhp8wt00kcrmJ2+BVpuID3VKJlflegUbBZ+aW7QZ69vPGB68J5EQrPpvA== 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=mj+3g8gsYmdsJVEZQioeUIr39mWjDFPABlGTmcn6QlA=; b=aYqBDp5n4VXMZKkWZagDgASMSnuQD7Kz0ATt0OkXAduAN6hpr//IyJ2/71DEPnoOp6PgMABDgpkfBH5xMY/lpU2Owi9rr+8625ykQpWl94xGvF4Huq41bjoy785a2xeYPbAKei7x1yFdzhRk7myZ4JmX8j4TExnYLsVbA+JyOEheDf1J7yZQZAg6uWgOquDp2FmExMJM2kBDMn30/w3o6Pc3SuhbAOY5dAy1bKY2zr2AByRpDHVTpXgc84+/WgPmTOajrn9rUkyHoi3dB7hYDLxrV9L++6vfLAN5Pj5JreCvL25V0o35saxOPFwHA0An/JNMeJABLlODFOW30qdSZQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=HOTMAIL.DE; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=mj+3g8gsYmdsJVEZQioeUIr39mWjDFPABlGTmcn6QlA=; b=qGOMGzM0ZKRFxDQEnqiRjoQMyAXxQsP+8zREu72pdVWqviEakpTSQsKaqtbCXV9+PfSeY9S+eTEj9L+nLYte+l6RQGaq7eIwI9nmk9eCEeA0NWmHBC4jSa+Cm0lSXIPiXedfW21pnQhuJiHzxno629hQc1mvb7ISkDEdZwqlr3+X/rsQp3h60PYjQM4isW9l2+uSZwSPRuO/4xGDa1wwSifowWg4LFCr9HjcgBARiphELu+3unuYhZqIOlBvm3Fjcv148e3OtGZiiaqa31XdVH5n/zQCz5iWEk4+sL93IEIN4nLwRsUS/3ZPWyV4jaTJ4lK2/lWQluw/etfVVyv9gw== Received: from AS8P193MB1285.EURP193.PROD.OUTLOOK.COM (2603:10a6:20b:333::21) by AS8P193MB1222.EURP193.PROD.OUTLOOK.COM (2603:10a6:20b:319::21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7677.30; Mon, 17 Jun 2024 10:41:20 +0000 Received: from AS8P193MB1285.EURP193.PROD.OUTLOOK.COM ([fe80::e65d:5dd2:5662:c61f]) by AS8P193MB1285.EURP193.PROD.OUTLOOK.COM ([fe80::e65d:5dd2:5662:c61f%4]) with mapi id 15.20.7677.030; Mon, 17 Jun 2024 10:41:20 +0000 Message-ID: Date: Mon, 17 Jun 2024 12:43:51 +0200 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 2/2] sim: riscv: Simplify the signed div by -1 code To: Andrew Burgess , "gdb-patches@sourceware.org" References: <87frtja1kr.fsf@redhat.com> <875xu86j3t.fsf@redhat.com> Content-Language: en-US From: Bernd Edlinger In-Reply-To: <875xu86j3t.fsf@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-TMN: [2rx/TYv/GN5g2EDSOY15r+4Ofa9fT1xkQ/bgcIy3oRhSRhVpGkT90Uf6GXlyrJfx] X-ClientProxiedBy: BE1P281CA0281.DEUP281.PROD.OUTLOOK.COM (2603:10a6:b10:84::14) To AS8P193MB1285.EURP193.PROD.OUTLOOK.COM (2603:10a6:20b:333::21) X-Microsoft-Original-Message-ID: <03cec868-6b08-4bbf-afe4-afa73e099147@hotmail.de> MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: AS8P193MB1285:EE_|AS8P193MB1222:EE_ X-MS-Office365-Filtering-Correlation-Id: 4c17e090-f4cf-4159-4a93-08dc8eba0629 X-Microsoft-Antispam: BCL:0;ARA:14566002|461199025|3412199022|4302099010|440099025|1602099009; X-Microsoft-Antispam-Message-Info: Z5bBGhYswIrEemldep2Cl/4glOzy+ziE8bjFDGp/hJ7uyRRoFO3ougG4tBJjzwEB6DBx2aQJwv5hxpcsqfByEAJa9X0JUn5i1ujoSBxklpO7LGb3aMJ07+47Xw0v0MTFd5k9Ge9vfvUUcZckj4bHV0HR/boRc/1axwYwG/uV3Mrs7uEGsLYKd0Wk98IDtyvd4BKBQRzXSBpl7BmOyabPCv+de1tf/pVLnvjKSi7bANnEaybI/5SE3XtShmLzUlWNh/La8SZiEbCgEkX4Rd8jKYHoPC3GagX6r/VSupRIci7AtbfIGcwTA35AZswuE1e1iLR5IXaKbb3atu4c3kKZL0YdNp+q8WhU7wE7/9G39qXnEl8LWbRdFyz0JincarIKoIFADkrod10imCTzTAOLqF9bmYR8gW++01lxqWjzCJEwin0Fd9t9K1l0NLSJHeCqU7L36yqOuIgjcP+G9AzH/dTXOyBk708/PKUhu+HNK37YNoGC77QKvor6HSsE8I0kXIL16D5Mq04HW59VIX6/2dPNH9wAsKYUG22lUzaIpRWPJz6kDQIBDZxX61p7f/FLsbSpQk0Jq6hkUXsRI3m7WD3lasAI8OVA/vcTs5mqVV9t4fCVcA8VGWHWk0DdbbVZPtrhO02ZhG72rbX7SWktbudzTQkhnjxtG8+Jl9neCUG3ASmGirXRQvFh2dn0p1IH X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?VHNieTdWaWlRZ1R4dGJmdjZmeGxPWTBGTDM4S2ZVMkVhQ0pyczgyNFZaRFQ2?= =?utf-8?B?cno4ZSt1UDFzUjRhZkF0b2JmcnZlU1VUR1hHMURpeVdmcW9yU0dCNjRSU2VQ?= =?utf-8?B?TUZXSmNjcHdldzBXRmtyWnIwVEZkOE1xT25CZWRmZCttNk1RdHJNZ1RtZXFY?= =?utf-8?B?ejBkNzVSOFBLMHlDUnB4ZHpnU3ZkUTc0allZRnErRzBEaE4yK1lTd0lRVkdr?= =?utf-8?B?S2c3RW5Eay9DT1h6UDNZZG9uTFBuekJETVFGSVJZdU5CWE1BdFRkRnBSMEZu?= =?utf-8?B?QUFNVUJuaDk3WVBERi9nbjR5OTJRbDdzWnV0eWRsSVhhUVJmejFpWlJiOU1h?= =?utf-8?B?L3VTZjljSzRGZHBzUXBCdGQvYzFHUGR5Q1E5KzJXeVA2VUh1bmdKNTQxbS82?= =?utf-8?B?TEJqalhNNnZWcWxvVTY4NG9TbklRVjVmaEtqSUgvcXlqVFdFZlpSZTZkdkNx?= =?utf-8?B?QTZPMC9OL29uc0dhekVLeEZ2YjZRSXczOGkwV2ZXNDVHcVVONXFXd09nMis1?= =?utf-8?B?NjVCNUU4R1M1Y1ZwV2txcG5PU2JJc05QQWNhWXVJVi9FL0dZUUdETTg1dXEx?= =?utf-8?B?SjNXZzVqS2pmejBJemZTZHdFRHVHOFk5Vkt2R0NPTktsdWR2bVdnbG9lZkQ0?= =?utf-8?B?M0NKZjU3SGQ2eUhVWlFFejloV3NFRG9yLzhuWnh5U2ROc3dUL0VxODdDL2Y0?= =?utf-8?B?NTdPTFdlaW1BMHd5QjdNSldsV1ZZOGNZaEQ5Rmc0eTVuZmFxRjMwM2EwWXp0?= =?utf-8?B?YU1yMG11Q2dLQ2hrUU9GZFpWNzd6UkhSbXpwWTdtb1JPYVd2RGZkZlJWOTVY?= =?utf-8?B?YkJHMVVtZG9GN0ZwdGpVSG9nNjFBU0JicExOMjloN3ZzOGE1emdhY0FRUlpW?= =?utf-8?B?K2NwSE4wVEM4RG9ZNmVKN3doZWlXUFRhR0srWDFyUXYyTm90RHoraFRYNGxL?= =?utf-8?B?cHo4cGRMT3RsWnVvUW82aGM0M3BQOG9McUVlQjNGMGsyb1JpTWFzMDZzaGx0?= =?utf-8?B?WlFtU3pBeXRCMmFqVU1la0picDdaQ0RLQTlSNmI4UThrcWgxd1k0WnRlNzFV?= =?utf-8?B?b1VmcU5xWmlua3RZYXh4ejlVZFhCNUZ3L2RicHk2SU5UNnQ4US9qcyt3eDFl?= =?utf-8?B?OVpqdzR4U2dFek9pbmlNRzNmcXRhRDRrMVhVeU9XSDIwblJVVGFSRmZBUmFQ?= =?utf-8?B?dy9CWVBDak5nK3BKZFBiTU1QS0FUbWRHL21qVnNySGtSTTRJRlVEd1FocFhG?= =?utf-8?B?WFFiWnAyRytFcFR4OHp4UzlMUWl4TkxUQllyZVlPYXg4amN5QkdhM2NaUTJa?= =?utf-8?B?WUFsTDNpeHFvUGo5UXNYdTNIOGpUSVltNmRMNW1ad0pyVXhKQmR4MmVZLzdn?= =?utf-8?B?OExjV2l6VkM2Nm4rZ2VrdkFBVStvZ2h2emNEekdha2hValBSVVdtM2IxUGNo?= =?utf-8?B?YVorVWRPMVpyaGllRzBzeGJQUDdCM2lSZGF2eUtObERVQlQwRzhRTy9YVVc4?= =?utf-8?B?RlZZa1pVeTJUMHY2VWZRV2ozUk9YbGRtWjNqa042eUhibWNTODRTdk0zazU1?= =?utf-8?B?QzkxNVg3UGhaZHVSQTd1ZFM1R3FTdGZHNUxNYlZENXR3cGh4NklhSlhYR3pW?= =?utf-8?B?OEpyQWNFV2w5Um15QmNwdCs2dkpzWG9BV3IxNHp3dTlhN3RPTlhVeEVIMGJZ?= =?utf-8?B?L2xocHFEUENyNFQ4RCsyeTVQajVYcVNFSnBrV2NJcnQwM1djWVl5TU93OUtU?= =?utf-8?Q?tIgmZEZEM4Lh8qyro9diCB2eXVHhbNiVy7g8Q0P?= X-OriginatorOrg: sct-15-20-4755-11-msonline-outlook-80ceb.templateTenant X-MS-Exchange-CrossTenant-Network-Message-Id: 4c17e090-f4cf-4159-4a93-08dc8eba0629 X-MS-Exchange-CrossTenant-AuthSource: AS8P193MB1285.EURP193.PROD.OUTLOOK.COM X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Jun 2024 10:41:20.0727 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000 X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS8P193MB1222 X-Spam-Status: No, score=-12.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 6/17/24 10:58, Andrew Burgess wrote: > Bernd Edlinger writes: > >> On 6/11/24 18:25, Andrew Burgess wrote: >>> Bernd Edlinger writes: >>> >>>> This uses the idea from the previous patch to >>>> simplify the code for non-overflowing signed >>>> divisions by -1. This is no bug-fix but it >>>> simplifies the code and avoids some unnecessary >>>> branches. >>>> --- >>>> sim/riscv/sim-main.c | 10 ++++------ >>>> 1 file changed, 4 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/sim/riscv/sim-main.c b/sim/riscv/sim-main.c >>>> index 515ff835223..e4b15b533ba 100644 >>>> --- a/sim/riscv/sim-main.c >>>> +++ b/sim/riscv/sim-main.c >>>> @@ -700,18 +700,16 @@ execute_m (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op) >>>> const char *rd_name = riscv_gpr_names_abi[rd]; >>>> const char *rs1_name = riscv_gpr_names_abi[rs1]; >>>> const char *rs2_name = riscv_gpr_names_abi[rs2]; >>>> - unsigned_word tmp, dividend_max; >>>> + unsigned_word tmp; >>>> sim_cia pc = riscv_cpu->pc + 4; >>>> >>>> - dividend_max = -((unsigned_word) 1 << (WITH_TARGET_WORD_BITSIZE - 1)); >>> >>> I really don't understand what this was trying to calculate. It's not >>> the max value of an unsigned_word. Nor is it the bit representation of >>> the maximum signed_word value... but you're removing it, so I guess I >>> can not worry about that :) >>> >> >> This is the largest negative dividend, either -2**31=-2147483648 or -2**63=-9223372036854775808 > > I guess my definition of 'max' and 'min' differ from whoever wrote this > code! I'd describe those values as 'dividend_min'. But as the code is > going away it's really not important. > Ack. >> and dividing this value by any non-negative value is always fine, except for the divisor=-1, >> because the result will overflow, and can therefore cause problems. > > Ah, got you. This for sure should have a comment explaining the reasoning. > >> >> But by my reasoning I found a simple solution that handles the divisor -1 for all dividends >> correctly and more efficiently, by simply computing -(unsigned)x instead of x/-1. >> >>>> - >>>> switch (op->match) >>>> { >>>> case MATCH_DIV: >>>> TRACE_INSN (cpu, "div %s, %s, %s; // %s = %s / %s", >>>> rd_name, rs1_name, rs2_name, rd_name, rs1_name, rs2_name); >>>> - if (riscv_cpu->regs[rs1] == dividend_max && riscv_cpu->regs[rs2] == -1) >>>> - tmp = dividend_max; >>>> + if (riscv_cpu->regs[rs2] == -1) >>>> + tmp = -riscv_cpu->regs[rs1]; >>> >>> As with the previous commit, can't we drop this whole if check and block? >>> >> >> No in this case the division would indeed overflow on the division dividend_max/-1. > > Agree. Just add a comment explaining this please. > Okay, I'll add this comment: --- a/sim/riscv/sim-main.c +++ b/sim/riscv/sim-main.c @@ -766,6 +766,9 @@ execute_m (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op) case MATCH_DIV: TRACE_INSN (cpu, "div %s, %s, %s; // %s = %s / %s", rd_name, rs1_name, rs2_name, rd_name, rs1_name, rs2_name); + /* Dividing by -1 can potentially overflow, and has therefore + to be done separately. Note that the sign change operation + is done on an unsigned value and is therefore safe to do. */ if (riscv_cpu->regs[rs2] == -1) tmp = -riscv_cpu->regs[rs1]; else if (riscv_cpu->regs[rs2]) How do you like that explanation? >> >>> I think we should add some tests for this too. The test I proposed in >>> the previous commit can be modified to give us div.s and rem.s tests. >>> >> >> Well, since this is just refactoring, and there is already very good test >> coverage in the gcc testsuite, I would not do that here, but instead > > The simulator should have tests in the simulator tree so I think we need > to add tests. > > As I've already supplied a rough framework for the previous commit, > which I believe you're going to polish for that test, all you need to do > here is copy that framework, change divw to div, and fill in a few > interesting test cases, so it shouldn't take long. > Okay, if you like, I will add some more tests, the only interesting thing is how they made the result of divide by zero defined, but this patch did not touch that code, yet it may be worth to have some test coverage for that else branch here. Thanks Bernd. >> I think the overflow bug in the mulh instruction really deserves a test case: >> >> [PATCH] sim: riscv: Fix undefined behaviour in mulh and similar >> https://sourceware.org/pipermail/gdb-patches/2024-April/208616.html >> >> So I will post an update for this one with a test case that demonstrates >> the possible mis-compilation due to a compiler optimization based on the >> assumption that an undefined behaviour may never happen, which caused >> user visible effects in this case. > > Great, the more tests the better. > > Thanks, > Andrew >