From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ot1-x342.google.com (mail-ot1-x342.google.com [IPv6:2607:f8b0:4864:20::342]) by sourceware.org (Postfix) with ESMTPS id E3D8F3857C62 for ; Fri, 28 Aug 2020 15:36:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org E3D8F3857C62 Received: by mail-ot1-x342.google.com with SMTP id k2so1193119ots.4 for ; Fri, 28 Aug 2020 08:36:31 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=pcEN0/CA79WWIxSEtaijEoONoNYo86a/0lj461n1154=; b=ffAJbR8jw8BCtTTKM5AjWSehBwOStZfpIIXPC1kgYqIt0KsW1H/pk39bs+yJnfD5D6 0eJixvV75eJ73iY/WydGMxHew4QamhvptPVjhRY84b4zmY3iV9Fq80rFrktm8XwOnFuA 24KWyMbHuU4BlkSmufB93ToVFG4SvX7TQsIWMfXvFkwuAR/od+ugAVegfaGgrJnEhGon WjOINHIoppeoBayo+BcOB/WKVmIvz7q8idg1e2BxcHVZsJwmjXF5/8I4ohvGtFUWLCkE hFXSsM3/8MiItk8NaptPNOpKi6lpKnErrvL/lKEPKagE4MYL6c+dT8zsvTsDMYh7l90b 3TJQ== X-Gm-Message-State: AOAM530IfbFcnSFpOO3porDRzA7vbWdGooNsE8rrrAa2ZXOUFC7E2qDq w73YssnOAk3h2zi1VP0jmB3BgntXkxcJUAuA69/WEA== X-Google-Smtp-Source: ABdhPJzmMqZn8zmPwgnWbn616FjdYrzg9uO5W3eac9d6x0WkfHeWa0IlHTEfvJDePK+BasuJSqPwNunvw6E621yjc10= X-Received: by 2002:a05:6830:1552:: with SMTP id l18mr1491568otp.269.1598628991027; Fri, 28 Aug 2020 08:36:31 -0700 (PDT) MIME-Version: 1.0 References: <1598534847-8544-1-git-send-email-christophe.lyon@linaro.org> In-Reply-To: From: Christophe Lyon Date: Fri, 28 Aug 2020 17:36:20 +0200 Message-ID: Subject: Re: [PATCH] arm: Fix switch tables for thumb-1 with -mpure-code [PR96768] To: Richard Earnshaw Cc: gcc Patches Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-10.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, 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 15:36:33 -0000 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. >