Message ID | 20cefa11-9565-47f5-b1bc-bd90e8c428fc@gmail.com |
---|---|
State | New |
Headers |
Return-Path: <gdb-patches-bounces+patchwork=sourceware.org@sourceware.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 B51D5385828C for <patchwork@sourceware.org>; Sun, 26 Nov 2023 18:48:06 +0000 (GMT) X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from mail-pl1-x62b.google.com (mail-pl1-x62b.google.com [IPv6:2607:f8b0:4864:20::62b]) by sourceware.org (Postfix) with ESMTPS id EC2993858CD1 for <gdb-patches@sourceware.org>; Sun, 26 Nov 2023 18:47:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org EC2993858CD1 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org EC2993858CD1 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::62b ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1701024473; cv=none; b=gtUtNHiY5+XY4N4K9h5qCW6YcQBJljgWdmZiVsOmbTAs7qf17MkFyXkryHlpvNEO8swpnNEXOb69iQZFE2I3zeqxkz+Xyh225XysMXqPrIvg8x/8NFXLlBncqoEOUCkMIKOtr2QCywmcTJEk7yD7evjoSD7S251IoewduSU8GuM= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1701024473; c=relaxed/simple; bh=7g7+umEjaoowUq8BRPyf2XckFwpxZ2r+x9X+lqnjcC8=; h=DKIM-Signature:Message-ID:Date:MIME-Version:To:From:Subject; b=Q/TsQcZyiHn3JSphFSWjzaTCt5ODYtHr7sEHsZAOkP6G+A+1FFUU+mQPF55ftbGl2ZC+BW1pP6K8htRH9br6uDoOj3JEkXuhkVCSrQY+6XIxcJJeilKka7Yf0f9F//4KKn2A7UCyHoauqGAlivj9NtFM4y5vM369jc7rXhYPISo= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-pl1-x62b.google.com with SMTP id d9443c01a7336-1cfb2176150so8117355ad.3 for <gdb-patches@sourceware.org>; Sun, 26 Nov 2023 10:47:49 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1701024468; x=1701629268; darn=sourceware.org; h=subject:from:to:content-language:user-agent:mime-version:date :message-id:from:to:cc:subject:date:message-id:reply-to; bh=iRdk/IY9+Cg6JnSb/j2W2zH6Zuw1rXPXPfB2arT+3r8=; b=OrBsy6cf69fKCdkFK2cSuASuQfl73v8kBsCQo6lvmFsfGH3XEg5NeHusx6QUu+KHYU E5sU0BYM8YC4muCdd95Yxtif8isnPsTX67Llx/kT6ottsiIumz3mvTNdeI8/xr3XyI6A PUIRpkE38Y30eGjCDFdSI8gUzXpLsN+N5tkU9luydlQJSW2Jb4tDIGt43WTr+TCi2Pzh ni3J7+wLiNXhMialewzScm3NMCYnYNEXboW9gdEenwCze8v2jA/Nr6poRyXvSWh8wMKu UiXwAA5en/AfIKjt1u1zXVwQ1A2aoxaRI/6UMeTltb0EMbAR6hfihTliF/xKE1U8TWva yzBQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1701024468; x=1701629268; h=subject:from:to:content-language:user-agent:mime-version:date :message-id:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=iRdk/IY9+Cg6JnSb/j2W2zH6Zuw1rXPXPfB2arT+3r8=; b=V1Pb4yx5vr9S44p41bRjeEyAIR6pSEl8KaZLSwnja7FrEtEa4ZxBNhtUef7yehiyne UmxPQeUQN0Guh5D1kCr/WdaRmc+wC19V7iSZxdmZ+qZ4/ZxDgQT59fy5Fun36Iqf7E/K VgvMiWqa3grGC3oOGCxCioUkjwMIr9RBUaZrmko83fomaJIePkPfQgh7RBfSFmqH/Gd+ VFPw2aQDqkqtorWzpOAxS2UUbOrY2TAXBHKyRRRJrFociuhFSi1uU6/m2nJJ2OcFEJMS aAi7wtWATch/rQZY1XCURPvP+XahpaZGDXzXpNBRgF+svme27mlkbU4t5pE35QxCj8DY Dxag== X-Gm-Message-State: AOJu0YwJijZe16boMtZXeS+WxdE5Hz5Ji7IyviXXHmvcIWGilhjhwS88 8vRQHjUCoQCFTAIizoGf+nKBBp214ewWcA== X-Google-Smtp-Source: AGHT+IFA0wD3LK1OokMl0wgs2P0xRYxR+ITFmiwO+wGVGk8FaazitMWt0idYrjGgBcvO+nQTyJOBVQ== X-Received: by 2002:a17:902:f685:b0:1cf:c9c3:e79d with SMTP id l5-20020a170902f68500b001cfc9c3e79dmr1449987plg.59.1701024467856; Sun, 26 Nov 2023 10:47:47 -0800 (PST) Received: from [172.31.0.109] ([136.36.130.248]) by smtp.gmail.com with ESMTPSA id w13-20020a170902e88d00b001cfc1a593f7sm1683836plg.217.2023.11.26.10.47.45 for <gdb-patches@sourceware.org> (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 26 Nov 2023 10:47:47 -0800 (PST) Content-Type: multipart/mixed; boundary="------------we4CZuEXOA5ZSfe0dqgpirJO" Message-ID: <20cefa11-9565-47f5-b1bc-bd90e8c428fc@gmail.com> Date: Sun, 26 Nov 2023 11:47:43 -0700 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: gdb-patches@sourceware.org From: Jeff Law <jeffreyalaw@gmail.com> Subject: Another fix for the mcore simulator X-Spam-Status: No, score=-8.3 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, SPF_HELO_NONE, 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 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list <gdb-patches.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=subscribe> Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org |
Series |
Another fix for the mcore simulator
|
|
Checks
Context | Check | Description |
---|---|---|
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 | fail | Patch failed to apply |
linaro-tcwg-bot/tcwg_gdb_build--master-arm | fail | Patch failed to apply |
Commit Message
Jeff Law
Nov. 26, 2023, 6:47 p.m. UTC
Here we go again. About a month ago a change to GCC's generic optimizers triggered a "regression" for mcore-elf. In simplest terms the change eliminated a zero extension and after a quick analysis it was pretty clear the zero extension should have made no difference in the visible behavior of the code. Naturally I suspected another problem with the mcore-elf simulator when run on a 64 bit host. I wasn't disappointed. This time it was the logical right shift instruction that was clearly wrong. This fixes around 500 more tests in the GCC testsuite and knocks about 40 minutes off its runtime -- it's now only about 2-3X slower than other comparable targets. Which is a massive improvement over where it was before I fixed the sext instructions. No doubt there's more 64 bit host issues in the mcore simulator. I can't justify chasing them all down preemptively. But I'll obviously continue to fault in fixes if/when the GCC testsuite shows a regression. OK for the trunk? jeff
Comments
>>>>> "Jeff" == Jeff Law <jeffreyalaw@gmail.com> writes:
Jeff> No doubt there's more 64 bit host issues in the mcore simulator. I
Jeff> can't justify chasing them all down preemptively. But I'll obviously
Jeff> continue to fault in fixes if/when the GCC testsuite shows a
Jeff> regression.
Jeff> OK for the trunk?
I don't have any issue with the patch, but I confess I don't understand
what the bug was or how the patch works.
Jeff> unsigned imm = IMM5;
Jeff> - unsigned long tmp = gr[RD];
Jeff> + uint32_t tmp = gr[RD];
IIUC gr is a uint32_t so this patch seems like it just removes a zero
extension and then a narrowing cast.
Anyway, if it helps, IMO that's enough.
Approved-By: Tom Tromey <tom@tromey.com>
Tom
On 11/27/23 13:06, Tom Tromey wrote: >>>>>> "Jeff" == Jeff Law <jeffreyalaw@gmail.com> writes: > > Jeff> No doubt there's more 64 bit host issues in the mcore simulator. I > Jeff> can't justify chasing them all down preemptively. But I'll obviously > Jeff> continue to fault in fixes if/when the GCC testsuite shows a > Jeff> regression. > > Jeff> OK for the trunk? > > I don't have any issue with the patch, but I confess I don't understand > what the bug was or how the patch works. > > Jeff> unsigned imm = IMM5; > Jeff> - unsigned long tmp = gr[RD]; > Jeff> + uint32_t tmp = gr[RD]; > > IIUC gr is a uint32_t so this patch seems like it just removes a zero > extension and then a narrowing cast. "gr" is actually a "int32_t", so it's signed. But even so, if it's stuffed into a "unsigned long" it's going to be zero extended. So I certainly see your point. Given there's some confusion here, I'll revert it out of my tester which should result in seeing some failures return and I can dive into the simulator again to extract the relevant state in the lsri/lsr instructions. jeff
>> IIUC gr is a uint32_t so this patch seems like it just removes a >> zero >> extension and then a narrowing cast. Jeff> "gr" is actually a "int32_t", so it's signed. Aha, say no more. Thank you. Jeff> Given there's some confusion here, I'll revert it out of my tester Jeff> which should result in seeing some failures return and I can dive into Jeff> the simulator again to extract the relevant state in the lsri/lsr Jeff> instructions. FWIW I think your patch is fine as-is. Tom
On 11/28/23 08:09, Tom Tromey wrote: >>> IIUC gr is a uint32_t so this patch seems like it just removes a >>> zero >>> extension and then a narrowing cast. > > Jeff> "gr" is actually a "int32_t", so it's signed. > > Aha, say no more. Thank you. So just to close the loop here. As I noted "gr" is an int32_t. So in the old code when we load that into an unsigned long, we get a sign bit extension: Breakpoint 3, step_once (cpu=0x555555674860, sd=<optimized out>) at /home/jlaw/test/binutils-gdb/sim/mcore/interp.c:1062 1062 unsigned imm = IMM5; (gdb) p/x ((struct mcore_sim_cpu *)cpu.arch_data).active_gregs[5] $40 = 0x9abcdef0 (gdb) n 1063 unsigned long tmp = gr[RD]; (gdb) 1064 if (imm == 0) (gdb) p/x tmp $41 = 0xffffffff9abcdef0 And when we shift that value right, those 1's in bits 32..64 get shifted down into the result. There's actually two instances of this exact same problem. One for lsr and the other for lsrc,lsri. So if there's no objection I'd like to fix both at the same time. Cheers, Jeff
>>>>> "Jeff" == Jeff Law <jeffreyalaw@gmail.com> writes:
Jeff> There's actually two instances of this exact same problem. One for
Jeff> lsr and the other for lsrc,lsri. So if there's no objection I'd like
Jeff> to fix both at the same time.
Totally fine, thank you.
Tom
On 11/30/23 20:56, Tom Tromey wrote: >>>>>> "Jeff" == Jeff Law <jeffreyalaw@gmail.com> writes: > > Jeff> There's actually two instances of this exact same problem. One for > Jeff> lsr and the other for lsrc,lsri. So if there's no objection I'd like > Jeff> to fix both at the same time. > > Totally fine, thank you. Done. Next up, speed up the H8 simulator by 2X or more. Linear searching large arrays is bad bad bad :( jeff
diff --git a/sim/mcore/.interp.c.swp b/sim/mcore/.interp.c.swp new file mode 100644 index 00000000000..a11d2df9077 Binary files /dev/null and b/sim/mcore/.interp.c.swp differ diff --git a/sim/mcore/interp.c b/sim/mcore/interp.c index 48d9ff8645a..0bbb2c399b2 100644 --- a/sim/mcore/interp.c +++ b/sim/mcore/interp.c @@ -1060,7 +1060,7 @@ step_once (SIM_DESC sd, SIM_CPU *cpu) case 0x3E: case 0x3F: /* lsrc, lsri */ { unsigned imm = IMM5; - unsigned long tmp = gr[RD]; + uint32_t tmp = gr[RD]; if (imm == 0) { NEW_C (tmp); diff --git a/sim/testsuite/mcore/lsri.s b/sim/testsuite/mcore/lsri.s new file mode 100644 index 00000000000..fe15213d6b0 --- /dev/null +++ b/sim/testsuite/mcore/lsri.s @@ -0,0 +1,21 @@ +# check that sext.b/sext.h work correctly +# mach: mcore + +.include "testutils.inc" + + start + # Construct -1 + bmaski r2, 32 + + # logical shift right by 24 + lsri r2, 24 + + # Construct 255 + bmaski r1, 8 + + # Compare them, they should be equal + cmpne r2,r1 + jbt .L1 + pass +.L1: + fail