public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Matthew Fortune <Matthew.Fortune@imgtec.com>
To: Richard Sandiford <rdsandiford@googlemail.com>,
	Vladimir Makarov	<vmakarov@redhat.com>
Cc: Robert Suchanek <Robert.Suchanek@imgtec.com>,
	"gcc-patches@gcc.gnu.org"	<gcc-patches@gcc.gnu.org>,
	Kyrill Tkachov <kyrylo.tkachov@arm.com>
Subject: RE: [RFC][PATCH][MIPS] Patch to enable LRA for MIPS backend
Date: Fri, 09 May 2014 22:58:00 -0000	[thread overview]
Message-ID: <6D39441BF12EF246A7ABCE6654B0235352881A@LEMAIL01.le.imgtec.org> (raw)
In-Reply-To: <87mweuww37.fsf@talisman.default>

Richard/Vlad,

Richard Sandiford <rdsandiford@googlemail.com> writes:
> Kyrill Tkachov <kyrylo.tkachov@arm.com> writes:
> > On 03/05/14 20:21, Richard Sandiford wrote:

...snip...

> > Hi all,
> > This caused some testsuite failures on arm:
> > FAIL: gcc.target/arm/vfp-ldmdbd.c scan-assembler fldmdbd
> > FAIL: gcc.target/arm/vfp-ldmdbs.c scan-assembler fldmdbs
> > FAIL: gcc.target/arm/vfp-ldmiad.c scan-assembler fldmiad
> > FAIL: gcc.target/arm/vfp-ldmias.c scan-assembler fldmias
> >
> >  From the vfp-ldmdbd.c test this patch changed the codegen from:
> >      fldmdbd    r5!, {d7}
> >
> > into
> >      sub    r5, r5, #8
> >      fldd    d7, [r5]
> >
> > Which broke the test.
> 
> Sorry for the breakage.  I've reverted the patch for now and will send a
> fixed version when I have time.

The problem appears to lie with the new satisfies_memory_constraint_p
which is passing just the address to valid_address_p but the constraint
is a memory constraint (which wants the mem to be retained).

One option is to re-construct the MEM using the address_info provided
to valid_address_p. This has mode, address space and whether it was
actually a MEM to start with so there is enough information. 

Another issue noticed while looking at this:
process_address does not seem to make an attempt to check for
EXTRA_MEMORY_CONSTRAINT even though it does decompose memory addresses.
For the lea address case then the address is checked with valid_address_p.
This seems inconsistent, is it intentional?

The patch below on top of Richard's addresses both problems and for the
fldmdbd test gets the correct output. I haven't done any more testing
than that though. I suspect there may be a better approach to achieve
the same effect but at least this shows what is wrong with the original
change.

Regards,
Matthew

diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c
index f59bf55..22bb273 100644
--- a/gcc/lra-constraints.c
+++ b/gcc/lra-constraints.c
@@ -348,6 +348,9 @@ valid_address_p (struct address_info *ad, const char *constraint = 0)
   rtx saved_index_reg = NULL_RTX;
   rtx *base_term = strip_subreg (ad->base_term);
   rtx *index_term = strip_subreg (ad->index_term);
+#ifdef EXTRA_CONSTRAINT_STR
+  rtx orig_op = NULL_RTX;
+#endif
   if (base_term != NULL)
     {
       saved_base_reg = *base_term;
@@ -360,9 +363,18 @@ valid_address_p (struct address_info *ad, const char *constraint = 0)
       saved_index_reg = *index_term;
       lra_eliminate_reg_if_possible (index_term);
     }
+#ifdef EXTRA_CONSTRAINT_STR
+  if (ad->addr_outer_code == MEM)
+    {
+      orig_op = gen_rtx_MEM (ad->mode, *ad->outer);
+      MEM_ADDR_SPACE (orig_op) = ad->as;
+    }
+  else
+    orig_op = *ad->outer;
+#endif
   bool ok_p = (constraint
 #ifdef EXTRA_CONSTRAINT_STR
-	       ? EXTRA_CONSTRAINT_STR (*ad->outer, constraint[0], constraint)
+	       ? EXTRA_CONSTRAINT_STR (orig_op, constraint[0], constraint)
 #else
 	       ? false
 #endif
@@ -2865,7 +2877,8 @@ process_address (int nop, rtx *before, rtx *after)
   /* Target hooks sometimes reject extra constraint addresses -- use
      EXTRA_CONSTRAINT_STR for the validation.  */
   if (constraint[0] != 'p'
-      && EXTRA_ADDRESS_CONSTRAINT (constraint[0], constraint)
+      && (EXTRA_ADDRESS_CONSTRAINT (constraint[0], constraint)
+	  || EXTRA_MEMORY_CONSTRAINT (constraint[0], constraint))
       && valid_address_p (&ad, constraint))
     return change_p;
 #endif

  reply	other threads:[~2014-05-09 22:58 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-03-29  1:27 Robert Suchanek
2014-03-29 11:24 ` Richard Sandiford
2014-04-09 10:00   ` Robert Suchanek
2014-04-09 21:24     ` Richard Sandiford
2014-04-10 20:29       ` Richard Sandiford
2014-04-14 11:13       ` Robert Suchanek
2014-04-15 21:12         ` Richard Sandiford
2014-04-16 21:10           ` Robert Suchanek
2014-04-21 13:01             ` Richard Sandiford
2014-04-23 13:34               ` Robert Suchanek
2014-04-23 14:10                 ` Richard Sandiford
2014-04-23 15:34                   ` Robert Suchanek
2014-04-23 15:33               ` Vladimir Makarov
2014-05-03 19:21                 ` Richard Sandiford
2014-05-06 14:06                   ` Kyrill Tkachov
2014-05-06 19:22                     ` Richard Sandiford
2014-05-09 22:58                       ` Matthew Fortune [this message]
2014-05-10 18:44                         ` Richard Sandiford
2014-05-14 13:24                           ` Robert Suchanek
2014-05-15 21:05                             ` Robert Suchanek
2014-05-15 21:34                             ` Richard Sandiford
2014-05-16 21:05                               ` Robert Suchanek
2014-05-18 19:38                           ` Richard Sandiford
2014-06-02 19:37                             ` RFA: Make LRA temporarily eliminate addresses before testing constraints Richard Sandiford
2014-06-03 20:50                               ` Vladimir Makarov
2014-06-04 17:45                                 ` Richard Sandiford
2014-06-11 11:30                                   ` Robert Suchanek
2014-06-16 16:12                                   ` Robert Suchanek
2014-06-16 18:08                                     ` Vladimir Makarov
2014-06-18 20:52                                       ` Matthew Fortune
2014-03-29 14:08 ` [RFC][PATCH][MIPS] Patch to enable LRA for MIPS backend Jakub Jelinek

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=6D39441BF12EF246A7ABCE6654B0235352881A@LEMAIL01.le.imgtec.org \
    --to=matthew.fortune@imgtec.com \
    --cc=Robert.Suchanek@imgtec.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=kyrylo.tkachov@arm.com \
    --cc=rdsandiford@googlemail.com \
    --cc=vmakarov@redhat.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).