[PATCH] PowerPC VLE port

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] PowerPC VLE port

James Lemke
Attached are the patches for this gcc port.

On a recent checkout (r191027) I have run the DejaGNU suite with no new
failures for binutils, gas, ld, gcc, g++, gfortran.  A bootstrap is in
progress.

Comments?
OK to commit?

Thanks, Jim.

--
Jim Lemke
Mentor Graphics / CodeSourcery
Orillia Ontario

vle-gcc-ChangeLog.txt (35K) Download Attachment
vle-gcc-0905c.diff (248K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PowerPC VLE port

Andrew Pinski-2
On Thu, Sep 6, 2012 at 2:55 PM, James Lemke <[hidden email]> wrote:
> Attached are the patches for this gcc port.
>
> On a recent checkout (r191027) I have run the DejaGNU suite with no new
> failures for binutils, gas, ld, gcc, g++, gfortran.  A bootstrap is in
> progress.

Could you explain why you are changing system.h ?
Also seems like TARGET_VLE_ISEL should not be needed TARGET_ISEL is
always set for VLE targets.

Thanks,
Andrew

>
> Comments?
> OK to commit?
>
> Thanks, Jim.
>
> --
> Jim Lemke
> Mentor Graphics / CodeSourcery
> Orillia Ontario
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PowerPC VLE port

Maciej W. Rozycki-4
On Thu, 6 Sep 2012, Andrew Pinski wrote:

> Could you explain why you are changing system.h ?
> Also seems like TARGET_VLE_ISEL should not be needed TARGET_ISEL is
> always set for VLE targets.

 You mean this:

+  POWERPC_E200_MASK = MASK_VLE | MASK_ISEL | MASK_MULTIPLE

?  Well, this just marks that the e200 processor supports ISEL regardless
of the mode selected (standard vs VLE).  Then with -mvle ISEL is supposed
to be enabled regardless of the processor setting in effect (ISEL is a
part of the base VLE instruction set, while it is optional in the standard
mode).

  Maciej
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PowerPC VLE port

Andrew Pinski-2
On Thu, Sep 6, 2012 at 3:39 PM, Maciej W. Rozycki
<[hidden email]> wrote:

> On Thu, 6 Sep 2012, Andrew Pinski wrote:
>
>> Could you explain why you are changing system.h ?
>> Also seems like TARGET_VLE_ISEL should not be needed TARGET_ISEL is
>> always set for VLE targets.
>
>  You mean this:
>
> +  POWERPC_E200_MASK = MASK_VLE | MASK_ISEL | MASK_MULTIPLE
>
> ?  Well, this just marks that the e200 processor supports ISEL regardless
> of the mode selected (standard vs VLE).  Then with -mvle ISEL is supposed
> to be enabled regardless of the processor setting in effect (ISEL is a
> part of the base VLE instruction set, while it is optional in the standard
> mode).

What I mean is set TARGET_ISEL to true when -mvle is supplied.

Thanks,
Andrew
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PowerPC VLE port

Maciej W. Rozycki-4
On Thu, 6 Sep 2012, Andrew Pinski wrote:

> >  You mean this:
> >
> > +  POWERPC_E200_MASK = MASK_VLE | MASK_ISEL | MASK_MULTIPLE
> >
> > ?  Well, this just marks that the e200 processor supports ISEL regardless
> > of the mode selected (standard vs VLE).  Then with -mvle ISEL is supposed
> > to be enabled regardless of the processor setting in effect (ISEL is a
> > part of the base VLE instruction set, while it is optional in the standard
> > mode).
>
> What I mean is set TARGET_ISEL to true when -mvle is supplied.

1. Will it work (switch back to -mno-isel) if -mno-vle is requested
   further on the command line?

2. Separating the settings will help when/if per-function VLE/non-VLE
   switching support is implemented, e.g. along the lines of
   attribute((mips16)) and attribute((nomips16)).

  Maciej
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PowerPC VLE port

Joseph Myers
In reply to this post by James Lemke
On Thu, 6 Sep 2012, James Lemke wrote:

> Attached are the patches for this gcc port.
>
> On a recent checkout (r191027) I have run the DejaGNU suite with no new
> failures for binutils, gas, ld, gcc, g++, gfortran.  A bootstrap is in
> progress.

The -t* options added duplicate -mcpu= options; the only existing
precedent appears to be arm-vxworks and I don't think the options are
appropriate for generic PowerPC target files (not specific to an OS port
such as VxWorks with its own special selection of multilibs).  Instead, it
would be better to make the -mcpu= options imply appropriate other
options.

They also aren't mentioned in invoke.texi at all; all new options need
documenting in invoke.texi.  The only change to invoke.texi is listing
-mvle and -mno-vle in the initial option summary.  The main section of
PowerPC options documentation needs the actual substantive documentation
of the semantics of -mvle added; just the summary isn't enough.

How did any target using eabivle.h manage to build when it uses
MASK_NEW_MNEMONICS, which no longer exists?  Maybe this separate target
isn't needed at all....

Wouldn't it be better for longlong.h to have actual support for VLE rather
than just disabling the present code, or is such support much harder to do
than the present code?  (Using built-in functions, e.g. __builtin_clz if
that expands inline for VLE, is fine.)

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PowerPC VLE port

James Lemke
In reply to this post by Andrew Pinski-2
On 09/06/2012 06:09 PM, Andrew Pinski wrote:
> Could you explain why you are changing system.h ?

That was a convenience to me at one point.
It should have been deleted from the patch set.

--
Jim Lemke
Mentor Graphics / CodeSourcery
Orillia Ontario,  +1-613-963-1073
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PowerPC VLE port

James Lemke
In reply to this post by Joseph Myers
On 09/06/2012 07:07 PM, Joseph S. Myers wrote:
> The -t* options added duplicate -mcpu= options; the only existing
> precedent appears to be arm-vxworks and I don't think the options are
> appropriate for generic PowerPC target files (not specific to an OS port
> such as VxWorks with its own special selection of multilibs).  Instead, it
> would be better to make the -mcpu= options imply appropriate other
> options.

Agreed.  I will remove the -t* options.

--
Jim Lemke
Mentor Graphics / CodeSourcery
Orillia Ontario,  +1-613-963-1073
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PowerPC VLE port

David Edelsohn-2
In reply to this post by James Lemke
Jim,

It is unfortunate that you did not discuss your plans to implement VLE
with me during the design phase.

This patch contains a lot of unnecessary, gratuitous changes in
addition to being very invasive.  It was not edited and cleaned
sufficiently before posting.  It has too much of a negative impact on
the current PowerPC port.  The patch is not going to be accepted in
its current form.

- David
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PowerPC VLE port

Nathan Sidwell-3
On 09/08/12 00:52, David Edelsohn wrote:

> This patch contains a lot of unnecessary, gratuitous changes in
> addition to being very invasive.  It was not edited and cleaned
> sufficiently before posting.  It has too much of a negative impact on
> the current PowerPC port.  The patch is not going to be accepted in
> its current form.

could you explain in more detail what you find unsatisfactory.  Thanks.

nathan

--
Nathan Sidwell
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PowerPC VLE port

James Lemke
In reply to this post by David Edelsohn-2
On 09/07/2012 07:52 PM, David Edelsohn wrote:
> This patch contains a lot of unnecessary, gratuitous changes in
> addition to being very invasive.  It was not edited and cleaned
> sufficiently before posting.  It has too much of a negative impact on
> the current PowerPC port.  The patch is not going to be accepted in
> its current form.

David,
What are your thoughts on how to move forward.

--
Jim Lemke
Mentor Graphics / CodeSourcery
Orillia Ontario,  +1-613-963-1073
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PowerPC VLE port

David Edelsohn-2
On Mon, Sep 10, 2012 at 5:28 PM, Maciej W. Rozycki
<[hidden email]> wrote:
> David,
>
>> The %c print_operand modifier was added by Aldy for a pattern that he
>> added in 2004 and removed the same year.  However, he did not remove
>> the modifier.
>
>  Indeed -- introduced with r80876 and then removed in r84775 -- thanks for
> digging out the history behind this code.

>  So here's a new change to discard the case altogether, along with a
> leftover comment from r80876 that should have been removed in r84775 too.
>
>  OK to apply?
>
> 2012-09-10  Maciej W. Rozycki  <[hidden email]>
>
>         gcc/
>         * config/rs6000/rs6000.c (print_operand) <'c'>: Remove.
>         * config/rs6000/spe.md: Remove a leftover comment.

Okay.

Thanks, David
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PowerPC VLE port

Segher Boessenkool
>> 2012-09-10  Maciej W. Rozycki  <[hidden email]>
>>
>>         gcc/
>>         * config/rs6000/rs6000.c (print_operand) <'c'>: Remove.
>>         * config/rs6000/spe.md: Remove a leftover comment.
>
> Okay.

This patch wasn't sent to gcc-patches -- can we see it please?


Segher

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PowerPC VLE port

Maciej W. Rozycki-4
On Tue, 11 Sep 2012, Segher Boessenkool wrote:

> > > 2012-09-10  Maciej W. Rozycki  <[hidden email]>
> > >
> > >         gcc/
> > >         * config/rs6000/rs6000.c (print_operand) <'c'>: Remove.
> > >         * config/rs6000/spe.md: Remove a leftover comment.
> >
> > Okay.
>
> This patch wasn't sent to gcc-patches -- can we see it please?

 Umm, I didn't notice a cc to gcc-patches was removed in the course of
discussion, sorry about that.  Here's the change concerned.

  Maciej

gcc-powerpc-print-operand-c.patch
Index: gcc/config/rs6000/spe.md
===================================================================
--- gcc/config/rs6000/spe.md (revision 191161)
+++ gcc/config/rs6000/spe.md (working copy)
@@ -2945,8 +2945,6 @@
   "mfspefscr %0"
   [(set_attr "type" "vecsimple")])
 
-;; FP comparison stuff.
-
 ;; Flip the GT bit.
 (define_insn "e500_flip_gt_bit"
   [(set (match_operand:CCFP 0 "cc_reg_operand" "=y")
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c (revision 191161)
+++ gcc/config/rs6000/rs6000.c (working copy)
@@ -14659,14 +14659,6 @@ print_operand (FILE *file, rtx x, int co
       /* %c is output_addr_const if a CONSTANT_ADDRESS_P, otherwise
  output_operand.  */
 
-    case 'c':
-      /* X is a CR register.  Print the number of the GT bit of the CR.  */
-      if (GET_CODE (x) != REG || ! CR_REGNO_P (REGNO (x)))
- output_operand_lossage ("invalid %%c value");
-      else
- fprintf (file, "%d", 4 * (REGNO (x) - CR0_REGNO) + 1);
-      return;
-
     case 'D':
       /* Like 'J' but get to the GT bit only.  */
       gcc_assert (REG_P (x));
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PowerPC VLE port

Maciej W. Rozycki-4
In reply to this post by David Edelsohn-2
On Tue, 11 Sep 2012, David Edelsohn wrote:

> > 2012-09-10  Maciej W. Rozycki  <[hidden email]>
> >
> >         gcc/
> >         * config/rs6000/rs6000.c (print_operand) <'c'>: Remove.
> >         * config/rs6000/spe.md: Remove a leftover comment.
>
> Okay.

 I have applied this change now, thanks for your review.

  Maciej