[PATCH] PR60822 (m68k, missing earlyclobber in extendplussidi)

[PATCH] PR60822 (m68k, missing earlyclobber in extendplussidi)

[Segher Boessenkool]

operand[0] has a subreg taken (as operand[3]), which is modified
before operand[1] is used.

Built succesfully but I'm not set up to run the testsuite, sorry.
It fixes the testcase of course.


gcc/ChangeLog:

2014-04-16  Segher Boessenkool  <segher@kernel.crashing.org>

	* config/m68k/m68k.md (extendplussidi): Add earlyclobber.

---
 gcc/config/m68k/m68k.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/config/m68k/m68k.md b/gcc/config/m68k/m68k.md
index e61048b..9e7f3e2 100644
--- a/gcc/config/m68k/m68k.md
+++ b/gcc/config/m68k/m68k.md
@@ -1869,7 +1869,7 @@ (define_insn "extendsidi2"
 ;; result of the SI tree to be in the lower register of the DI target
 
 (define_insn "extendplussidi"
-  [(set (match_operand:DI 0 "register_operand" "=d")
+  [(set (match_operand:DI 0 "register_operand" "=&d")
     (sign_extend:DI (plus:SI (match_operand:SI 1 "general_operand" "%rmn")
             (match_operand:SI 2 "general_operand" "rmn"))))]
   ""
-- 
1.8.1.4

Re: [PATCH] PR60822 (m68k, missing earlyclobber in extendplussidi)

[Jeff Law]

On 04/16/14 13:18, Segher Boessenkool wrote:
> operand[0] has a subreg taken (as operand[3]), which is modified
> before operand[1] is used.
>
> Built succesfully but I'm not set up to run the testsuite, sorry.
> It fixes the testcase of course.
>
>
> gcc/ChangeLog:
>
> 2014-04-16  Segher Boessenkool  <segher@kernel.crashing.org>
>
> 	* config/m68k/m68k.md (extendplussidi): Add earlyclobber.
But in the case where writing operand3 would overwrite operand1, 
shouldn't we have have used the true arm of this statement:

  if (GET_CODE (operands[1]) == REG
       && REGNO (operands[1]) == REGNO (operands[3]))
     output_asm_insn ("add%.l %2,%3", operands);
   else
     output_asm_insn ("move%.l %2,%3\;add%.l %1,%3", operands);

Looking at the .reload dump I see:


(insn 11 33 14 2 (set (reg:DI 0 %d0 [orig:47 D.1394 ] [47])
         (sign_extend:DI (plus:SI (mem:SI (plus:SI (reg/v/f:SI 8 %a0 
[orig:40 p ] [40])
                         (reg:SI 1 %d1 [44])) [3 p_4(D)->a+0 S4 A16])
                 (mem:SI (plus:SI (reg/v/f:SI 8 %a0 [orig:40 p ] [40])
                         (reg:SI 0 %d0 [45])) [3 p_4(D)->b+0 S4 A16])))) 
j.c:12 78 {extendplussidi}


Isn't the problem that operands 1 is a MEM which use the same register 
as operands 3 in the memory address?

ISTM either removing the memory constraint entirely, or splitting it off 
into a separate alternative and only earlyclobbering that alternative 
would be better.

Or am I missing something?

jeff

Re: [PATCH] PR60822 (m68k, missing earlyclobber in extendplussidi)

[segher]

On Wed, Apr 16, 2014 at 02:45:28PM -0600, Jeff Law wrote:
> Isn't the problem that operands 1 is a MEM which use the same register 
> as operands 3 in the memory address?

Yes, exactly.

> ISTM either removing the memory constraint entirely, or splitting it off 
> into a separate alternative and only earlyclobbering that alternative 
> would be better.
> 
> Or am I missing something?

No, that does seem better :-)

I tried both your suggestions; the first results in better code.  Here's
a new patch.  As before, it builds and fixes the testcase, but I didn't
run the testsuite (I have no emulator set up).

Thanks,


Segher



gcc/

PR target/60822
2014-04-16  Segher Boessenkool  <segher@kernel.crashing.org>

	* config/m68k/m68k.md (extendplussidi): Don't allow memory for
	operand 1.

---
 gcc/config/m68k/m68k.md | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/gcc/config/m68k/m68k.md b/gcc/config/m68k/m68k.md
index e61048b..57ba1a1 100644
--- a/gcc/config/m68k/m68k.md
+++ b/gcc/config/m68k/m68k.md
@@ -1868,9 +1868,12 @@ (define_insn "extendsidi2"
 ;; Maybe there is a way to make that the general case, by forcing the
 ;; result of the SI tree to be in the lower register of the DI target
 
+;; Don't allow two memory operands: it needs an earlyclobber and will
+;; result in worse code.
+
 (define_insn "extendplussidi"
   [(set (match_operand:DI 0 "register_operand" "=d")
-    (sign_extend:DI (plus:SI (match_operand:SI 1 "general_operand" "%rmn")
+    (sign_extend:DI (plus:SI (match_operand:SI 1 "nonmemory_operand" "%rn")
             (match_operand:SI 2 "general_operand" "rmn"))))]
   ""
 {
-- 
1.8.1.4

Re: [PATCH] PR60822 (m68k, missing earlyclobber in extendplussidi)

[Jeff Law]

On 04/16/14 18:20, segher@kernel.crashing.org wrote:
> On Wed, Apr 16, 2014 at 02:45:28PM -0600, Jeff Law wrote:
>> Isn't the problem that operands 1 is a MEM which use the same register
>> as operands 3 in the memory address?
>
> Yes, exactly.
>
>> ISTM either removing the memory constraint entirely, or splitting it off
>> into a separate alternative and only earlyclobbering that alternative
>> would be better.
>>
>> Or am I missing something?
>
> No, that does seem better :-)
>
> I tried both your suggestions; the first results in better code.  Here's
> a new patch.  As before, it builds and fixes the testcase, but I didn't
> run the testsuite (I have no emulator set up).
>
> Thanks,
>
>
> Segher
>
>
>
> gcc/
>
> PR target/60822
> 2014-04-16  Segher Boessenkool  <segher@kernel.crashing.org>
>
> 	* config/m68k/m68k.md (extendplussidi): Don't allow memory for
> 	operand 1.
Thanks.  I tweaked the comment and added the testcase to the regression 
suite and installed the fix on the trunk.

Jeff

Re: [PATCH] PR60822 (m68k, missing earlyclobber in extendplussidi)

[Hans-Peter Nilsson]

On Thu, 24 Apr 2014, Jeff Law wrote:
> On 04/16/14 18:20, segher@kernel.crashing.org wrote:
> > PR target/60822
> > 2014-04-16  Segher Boessenkool  <segher@kernel.crashing.org>
> >
> > 	* config/m68k/m68k.md (extendplussidi): Don't allow memory for
> > 	operand 1.
> Thanks.  I tweaked the comment and added the testcase to the regression suite
> and installed the fix on the trunk.

In the commit message:

Added:
    trunk/gcc/testsuite/gcc.c-torture/execute/pr60822.c
    trunk/gcc/testsuite/gcc.c-torture/execute/pr60822.x

I hope it's not too officious to remind people that dg- markup
can be used *even* in the c-torture test-suite these days;
gating in .x files is not necessary (since a few years, IIRC).

brgds, H-P

Re: [PATCH] PR60822 (m68k, missing earlyclobber in extendplussidi)

[Joseph S. Myers]

On Mon, 12 May 2014, Hans-Peter Nilsson wrote:

> On Thu, 24 Apr 2014, Jeff Law wrote:
> > On 04/16/14 18:20, segher@kernel.crashing.org wrote:
> > > PR target/60822
> > > 2014-04-16  Segher Boessenkool  <segher@kernel.crashing.org>
> > >
> > > 	* config/m68k/m68k.md (extendplussidi): Don't allow memory for
> > > 	operand 1.
> > Thanks.  I tweaked the comment and added the testcase to the regression suite
> > and installed the fix on the trunk.
> 
> In the commit message:
> 
> Added:
>     trunk/gcc/testsuite/gcc.c-torture/execute/pr60822.c
>     trunk/gcc/testsuite/gcc.c-torture/execute/pr60822.x
> 
> I hope it's not too officious to remind people that dg- markup
> can be used *even* in the c-torture test-suite these days;
> gating in .x files is not necessary (since a few years, IIRC).

I believe bug 20567 is still current: gcc.c-torture/execute ignores dg- 
directives and still needs .x files, until someone converts it to the dg 
harness.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] PR60822 (m68k, missing earlyclobber in extendplussidi)

[Hans-Peter Nilsson]

On Tue, 13 May 2014, Joseph S. Myers wrote:
> On Mon, 12 May 2014, Hans-Peter Nilsson wrote:
> > On Thu, 24 Apr 2014, Jeff Law wrote:
> > > On 04/16/14 18:20, segher@kernel.crashing.org wrote:
> > > > PR target/60822
> > > > 2014-04-16  Segher Boessenkool  <segher@kernel.crashing.org>
> > > >
> > > > 	* config/m68k/m68k.md (extendplussidi): Don't allow memory for
> > > > 	operand 1.
> > > Thanks.  I tweaked the comment and added the testcase to the regression suite
> > > and installed the fix on the trunk.
> > In the commit message:
> > Added:
> >     trunk/gcc/testsuite/gcc.c-torture/execute/pr60822.c
> >     trunk/gcc/testsuite/gcc.c-torture/execute/pr60822.x
> >
> > I hope it's not too officious to remind people that dg- markup
> > can be used *even* in the c-torture test-suite these days;
> > gating in .x files is not necessary (since a few years, IIRC).
>
> I believe bug 20567 is still current: gcc.c-torture/execute ignores dg-
> directives and still needs .x files, until someone converts it to the dg
> harness.

I didn't believe that to be correct, so I checked; it is.
To wit, removing the .x file and adding a top line
"/* { dg-require-effective-target int16 } */"
didn't stop the test from being executed for an "int32plus"
target.  Also, no sign of dg-anything in c-torture.exp.
Sorry for being wrong.

Let me instead suggest sticking to gcc.dg/torture for new tests.
There I'd be right. :)

brgds, H-P