diff mbox

PR60822 (m68k, missing earlyclobber in extendplussidi)

Message ID 20140417002018.GA25172@gate.crashing.org
State New
Headers show

Commit Message

Segher Boessenkool April 17, 2014, 12:20 a.m. UTC
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(-)

Comments

Jeff Law April 24, 2014, 8:45 p.m. UTC | #1
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
Hans-Peter Nilsson May 13, 2014, 2:05 a.m. UTC | #2
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
Joseph Myers May 13, 2014, 5:19 p.m. UTC | #3
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.
Hans-Peter Nilsson May 14, 2014, 1:31 a.m. UTC | #4
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
diff mbox

Patch

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"))))]
   ""
 {