From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id D4CF0393C86F for ; Fri, 28 Aug 2020 17:54:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org D4CF0393C86F Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=foss.arm.com Authentication-Results: sourceware.org; spf=none smtp.mailfrom=Richard.Earnshaw@foss.arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 5FFB81FB; Fri, 28 Aug 2020 10:54:27 -0700 (PDT) Received: from [192.168.1.19] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id CFAD63F66B; Fri, 28 Aug 2020 10:54:26 -0700 (PDT) Subject: Re: [PATCH] arm: Fix switch tables for thumb-1 with -mpure-code [PR96768] To: Christophe Lyon Cc: gcc Patches References: <1598534847-8544-1-git-send-email-christophe.lyon@linaro.org> From: Richard Earnshaw Message-ID: <015818e0-e5c1-9365-7cbc-190c97ba9b48@foss.arm.com> Date: Fri, 28 Aug 2020 18:54:17 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-GB Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3499.0 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, KAM_LAZY_DOMAIN_SECURITY, NICE_REPLY_A, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 28 Aug 2020 17:54:29 -0000 On 28/08/2020 16:36, Christophe Lyon via Gcc-patches wrote: > On Fri, 28 Aug 2020 at 16:27, Richard Earnshaw > wrote: >> >> On 28/08/2020 14:24, Christophe Lyon via Gcc-patches wrote: >>> On Fri, 28 Aug 2020 at 14:00, Richard Earnshaw >>> wrote: >>>> >>>> On 27/08/2020 14:27, Christophe Lyon via Gcc-patches wrote: >>>>> In comment 14 from PR94538, it was suggested to switch off jump tables >>>>> on thumb-1 cores when using -mpure-code, like we already do for thumb-2. >>>>> >>>>> This is what this patch does, and also restores the previous value of >>>>> CASE_VECTOR_PC_RELATIVE since it was not the right way of handling >>>>> this. >>>>> >>>>> It also adds a new test, since the existing no-casesi.c did not catch >>>>> this problem. >>>>> >>>>> Tested by running the whole testsuite with -mpure-code -mcpu=cortex-m0 >>>>> -mfloat-abi=soft, no regression and the new test passes (and fails >>>>> without the fix). >>>>> >>>>> 2020-08-27 Christophe Lyon >>>>> >>>>> gcc/ >>>>> * config/arm/arm.h (CASE_VECTOR_PC_RELATIVE): Remove condition on >>>>> -mpure-code. >>>>> * config/arm/thumb1.md (tablejump): Disable when -mpure-code. >>>>> >>>>> gcc/testsuite/ >>>>> * gcc.target/arm/pure-code/pr96768.c: New test. >>>>> --- >>>>> gcc/config/arm/arm.h | 5 ++--- >>>>> gcc/config/arm/thumb1.md | 2 +- >>>>> gcc/testsuite/gcc.target/arm/pure-code/pr96768.c | 21 +++++++++++++++++++++ >>>>> 3 files changed, 24 insertions(+), 4 deletions(-) >>>>> create mode 100644 gcc/testsuite/gcc.target/arm/pure-code/pr96768.c >>>>> >>>>> diff --git a/gcc/config/arm/arm.h b/gcc/config/arm/arm.h >>>>> index 3887c51..7d43721 100644 >>>>> --- a/gcc/config/arm/arm.h >>>>> +++ b/gcc/config/arm/arm.h >>>>> @@ -1975,10 +1975,9 @@ enum arm_auto_incmodes >>>>> for the index in the tablejump instruction. */ >>>>> #define CASE_VECTOR_MODE Pmode >>>>> >>>>> -#define CASE_VECTOR_PC_RELATIVE ((TARGET_THUMB2 \ >>>>> +#define CASE_VECTOR_PC_RELATIVE (TARGET_THUMB2 \ >>>>> || (TARGET_THUMB1 \ >>>>> - && (optimize_size || flag_pic))) \ >>>>> - && (!target_pure_code)) >>>>> + && (optimize_size || flag_pic))) >>>>> >>>>> >>>>> #define CASE_VECTOR_SHORTEN_MODE(min, max, body) \ >>>>> diff --git a/gcc/config/arm/thumb1.md b/gcc/config/arm/thumb1.md >>>>> index f0129db..602039e 100644 >>>>> --- a/gcc/config/arm/thumb1.md >>>>> +++ b/gcc/config/arm/thumb1.md >>>>> @@ -2012,7 +2012,7 @@ (define_insn "*epilogue_insns" >>>>> (define_expand "tablejump" >>>>> [(parallel [(set (pc) (match_operand:SI 0 "register_operand")) >>>>> (use (label_ref (match_operand 1 "" "")))])] >>>>> - "TARGET_THUMB1" >>>>> + "TARGET_THUMB1 && !target_pure_code" >>>>> " >>>>> if (flag_pic) >>>>> { >>>>> diff --git a/gcc/testsuite/gcc.target/arm/pure-code/pr96768.c b/gcc/testsuite/gcc.target/arm/pure-code/pr96768.c >>>>> new file mode 100644 >>>>> index 0000000..fd4cad5 >>>>> --- /dev/null >>>>> +++ b/gcc/testsuite/gcc.target/arm/pure-code/pr96768.c >>>>> @@ -0,0 +1,21 @@ >>>>> +/* { dg-do compile } */ >>>>> +/* { dg-options "-mpure-code" } */ >>>>> + >>>>> +int f2 (int x, int y) >>>>> +{ >>>>> + switch (x) >>>>> + { >>>>> + case 0: return y + 0; >>>>> + case 1: return y + 1; >>>>> + case 2: return y + 2; >>>>> + case 3: return y + 3; >>>>> + case 4: return y + 4; >>>>> + case 5: return y + 5; >>>>> + } >>>>> + return y; >>>>> +} >>>>> + >>>>> +/* We do not want any load from literal pool, but accept loads from r7 >>>>> + (frame pointer, used at -O0). */ >>>>> +/* { dg-final { scan-assembler-not "ldr\tr\\d+, \\\[r\[0-689\]+\\\]" } } */ >>>>> +/* { dg-final { scan-assembler "text,\"0x20000006\"" } } */ >>>>> >>>> >>>> Having switch tables is only a problem if they are embedded in the .text >>>> segment. But the case you show in the PR has them in the .rodata >>>> segment, so is this really necessary? >>>> >>> >>> Well, in the original PR94538, comment #14, Wilco said this was the best option. >>> >> >> If the choice were not really a choice (ie pure code could not use >> switch tables and still be pure) yes, it would be the case. But that >> isn't the case. > > OK thanks, that was my initial impression. > > So it seems this PR is actually not-a-bug/invalid?.... > > >> >> GCC already knows generally when using jump sequences is going to be >> better than switch tables and when tables are likely to be better. It >> can even produce a meld of the two when appropriate, it can even take >> into account whether or not we are optimizing for size. So we shouldn't >> be just ride rough-shod over those choices. Pure code doesn't really >> change the balance. > > > ... or should the PR be the other way around and request to improve how such > cases are handled for thumb2? (ie do not disable arn.md:casesi completely > with -mpure-code?) > > > > >> >> R. >> >>>> R. >> Might be better to reject this one and create a new enhancement PR. R.