public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Richard Sandiford <richard.sandiford@linaro.org>
To: Prakhar Bahuguna <prakhar.bahuguna@arm.com>
Cc: <gcc-patches@gcc.gnu.org>,  <nd@arm.com>,
	 <Richard.Earnshaw@arm.com>,  <Ramana.Radhakrishnan@arm.com>,
		<Kyrylo.Tkachov@foss.arm.com>
Subject: Re: [PATCH, GCC/ARM/gcc-7-branch] Backport PR71607
Date: Wed, 31 May 2017 08:25:00 -0000	[thread overview]
Message-ID: <87o9u9mo9v.fsf@linaro.org> (raw)
In-Reply-To: <20170525132626.t536vqs4yhhtvwyh@e107464-lin.cambridge.arm.com>	(Prakhar Bahuguna's message of "Thu, 25 May 2017 14:26:26 +0100")

Prakhar Bahuguna <prakhar.bahuguna@arm.com> writes:
> This patch tackles the issue reported in PR71607. This patch takes a different
> approach for disabling the creation of literal pools. Instead of disabling the
> patterns that would normally transform the rtl into actual literal pools, it
> disables the creation of this literal pool rtl by making the target hook
> TARGET_CANNOT_FORCE_CONST_MEM return true if arm_disable_literal_pool is true.
> I added patterns to split floating point constants for both SF and DFmode. A
> pattern to handle the addressing of label_refs had to be included as well since
> all "memory_operand" patterns are disabled when TARGET_CANNOT_FORCE_CONST_MEM
> returns true. Also the pattern for splitting 32-bit immediates had to be
> changed, it was not accepting unsigned 32-bit unsigned integers with the MSB
> set. I believe const_int_operand expects the mode of the operand to be set to
> VOIDmode and not SImode. I have only changed it in the patterns that were
> affecting this code, though I suggest looking into changing it in the rest of
> the ARM backend.

I couldn't see the const_int_operand bit in the attached patch, but:
const_int_operand *should* usually be used with the logical integer
mode, such as SImode or DImode.

const_ints are supposed to be stored in sign-extended form, so a 32-bit
integer with the MSB set should be 0xffffffff80000000|x instead of
0x80000000|x.  It's a bug if you have one where that isn't true.

In the patch it looks like this could come from:

> diff --git a/gcc/config/arm/vfp.md b/gcc/config/arm/vfp.md
> index befdea9edd9..d8f77e2ffe4 100644
> --- a/gcc/config/arm/vfp.md
> +++ b/gcc/config/arm/vfp.md
> @@ -2079,3 +2079,40 @@
>  ;; fmdhr et al (VFPv1)
>  ;; Support for xD (single precision only) variants.
>  ;; fmrrs, fmsrr
> +
> +;; Split an immediate DF move to two immediate SI moves.
> +(define_insn_and_split "no_literal_pool_df_immediate"
> +  [(set (match_operand:DF 0 "s_register_operand" "")
> +	(match_operand:DF 1 "const_double_operand" ""))]
> +  "TARGET_THUMB2 && arm_disable_literal_pool
> +  && !(TARGET_HARD_FLOAT && TARGET_VFP_DOUBLE
> +       && vfp3_const_double_rtx (operands[1]))"
> +  "#"
> +  "&& !reload_completed"
> +  [(set (subreg:SI (match_dup 1) 0) (match_dup 2))
> +   (set (subreg:SI (match_dup 1) 4) (match_dup 3))
> +   (set (match_dup 0) (match_dup 1))]
> +  "
> +  long buf[2];
> +  real_to_target (buf, CONST_DOUBLE_REAL_VALUE (operands[1]), DFmode);
> +  operands[2] = GEN_INT ((int) buf[0]);
> +  operands[3] = GEN_INT ((int) buf[1]);
> +  operands[1] = gen_reg_rtx (DFmode);
> +  ")
> +
> +;; Split an immediate SF move to one immediate SI move.
> +(define_insn_and_split "no_literal_pool_sf_immediate"
> +  [(set (match_operand:SF 0 "s_register_operand" "")
> +	(match_operand:SF 1 "const_double_operand" ""))]
> +  "TARGET_THUMB2 && arm_disable_literal_pool
> +  && !(TARGET_HARD_FLOAT && vfp3_const_double_rtx (operands[1]))"
> +  "#"
> +  "&& !reload_completed"
> +  [(set (subreg:SI (match_dup 1) 0) (match_dup 2))
> +   (set (match_dup 0) (match_dup 1))]
> +  "
> +  long buf;
> +  real_to_target (&buf, CONST_DOUBLE_REAL_VALUE (operands[1]), SFmode);
> +  operands[2] = GEN_INT ((int) buf);
> +  operands[1] = gen_reg_rtx (SFmode);
> +  ")

...these two splits, where the GEN_INTs should probably be:

  gen_int_mode (..., SImode);

instead.

Thanks,
Richard

  reply	other threads:[~2017-05-31  8:19 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-05-25 14:02 Prakhar Bahuguna
2017-05-31  8:25 ` Richard Sandiford [this message]
2017-05-31 13:11   ` Prakhar Bahuguna
2017-05-31 13:20     ` Richard Sandiford
2017-05-31 15:40       ` Prakhar Bahuguna
2017-06-01  6:16         ` Richard Sandiford
2017-06-01 10:21           ` Prakhar Bahuguna

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87o9u9mo9v.fsf@linaro.org \
    --to=richard.sandiford@linaro.org \
    --cc=Kyrylo.Tkachov@foss.arm.com \
    --cc=Ramana.Radhakrishnan@arm.com \
    --cc=Richard.Earnshaw@arm.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=nd@arm.com \
    --cc=prakhar.bahuguna@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).