Re: [PATCH] PowerPC VLE port
On 10/19/2012 02:52 PM, David Edelsohn wrote: How do you want to move forward with the VLE patch? Can you localize more of the changes? David, I have been distracted by other tasks. I expect to revisit VLE this week. However, I won't be able to invest much more time on VLE. I'll look at what else I can do. -- Jim Lemke Mentor Graphics / CodeSourcery Orillia Ontario, +1-613-963-1073
Re: [PATCH] PowerPC VLE port
Jim, How do you want to move forward with the VLE patch? Can you localize more of the changes? Thanks, David
Re: [PATCH] PowerPC VLE port
Jim, This version of the VLE support patch is an improvement, although I still am troubled by the number of changes to rs6000.md. And can you suggest any way to re-factor rs6000_rtx_costs()? Andrew suggested placing all of the patterns in vle.md independently and without my prompting. VLE really is more of a separate architecture that happens to share some similarities and mnemonics with PowerPC, not a variant of PowerPC. I agree with Andrew that the cleanup portions of the patch should be sent separately. I also am continuing to work with Mike Meissner on the target flags patch, which will affect this patch. Thanks, David
Re: [PATCH] PowerPC VLE port
On Wed, Oct 3, 2012 at 6:59 AM, James Lemke jwle...@mentor.com wrote:
Ping..
@@ -847,7 +1106,6 @@
(DEFAULT_ABI != ABI_AIX || SYMBOL_REF_FUNCTION_P
(op)
;; Return 1 if op is an operand that can be loaded via the GOT.
-;; or non-special register register field no cr0
(define_predicate got_operand
(match_code symbol_ref,const,label_ref))
Most likely should be submitted and committed (as obvious) separately.
@@ -6694,7 +6771,7 @@
switch (mode)
{
- case QImode:
+case QImode:
case HImode:
if (dest == NULL)
dest = gen_reg_rtx (mode);
Likewise.
@@ -14944,11 +15104,10 @@
return;
case 'Q':
- if (TARGET_MFCRF)
- fputc (',', file);
-/* FALLTHRU */
- else
+ if (! TARGET_MFCRF)
return;
+ fputc (',', file);
+ /* FALLTHRU */
case 'R':
/* X is a CR register. Print the mask for `mtcrf'. */
Likewise.
@@ -15893,7 +16071,7 @@
}
/* Return the string to output a conditional branch to LABEL, which is
- the operand number of the label, or -1 if the branch is really a
+ the operand template of the label, or NULL if the branch is really a
conditional return.
OP is the conditional expression. XEXP (OP, 0) is assumed to be a
This looks like it should be done separately also.
+bool
+valid_vle_sd4_field (rtx mem, enum machine_mode mode)
No comment before this function.
- (simple_return )])
+ (simple_return 1)])
Submitted separately it looks.
libgcc/longlong.h
Gets sync'd with glibc's version sometimes.
Also have you thought have just adding a vle.md for all the needed
patterns and disabling the patterns in rs6000.md for VLE and not using
%^/%+/%- ? I think that would have been a cleaner implementation of
vle than adding support for it to the current patterns. Also does not
have the maintenance issue of always having to check if a new pattern
needs the %^/%+/%-.
Thanks,
Andrew Pinski
Original Message
Subject: [PATCH] PowerPC VLE port
Date: Mon, 24 Sep 2012 21:44:02 -0400
From: James Lemke jwle...@codesourcery.com
To: GCC Patches gcc-patches@gcc.gnu.org
The initial patch for this port caused much comment. I have attached
an updated patch trying to address many of those points.
All comments are welcome. I would prefer comments are made on-list.
I have tried to simplify this patch by:
1) Removing as many compound tests as possible and now rely on feature
flags.
I have removed TARGET_VLE_ISEL, TARGET_VLE_MULTIPLE, TARGET_VLE_ISEL64
TARGET_MFCRF_NOVLE.
-mcpu now implies the following options:
-mcpu=e200z[0-2]: -mvle -misel -mno-mfcrf -mmultiple -msoft-float
-mcpu=e200z[367]: -mvle -misel -mno-mfcrf -mmultiple -mfloat-gprs=single \
-mspe=yes -mabi=spe
2) When gcc is configured for a non-VLE target, TARGET_VLE evaluates to 0
so that most VLE-specific compiler code is optimized away.
3) Separated all VLE-only items from rs6000.md to a new file, vle.md.
In the cases where there was strong commonality there is still VLE code in
rs6000.md.
On r191665 I have run the DejaGNU suite. A bootstrap is running.
Comments?
Jim.
--
Jim Lemke
Mentor Graphics / CodeSourcery
Orillia Ontario, +1-613-963-1073
Re: [PATCH] PowerPC VLE port
;; Return 1 if op is an operand that can be loaded via the GOT.
-;; or non-special register register field no cr0
(define_predicate got_operand
(match_code symbol_ref,const,label_ref))
Most likely should be submitted and committed (as obvious) separately.
Yes, will do.
switch (mode)
{
- case QImode:
+case QImode:
case HImode:
if (dest == NULL)
dest = gen_reg_rtx (mode);
Likewise.
Will do.
case 'Q':
- if (TARGET_MFCRF)
- fputc (',', file);
-/* FALLTHRU */
- else
+ if (! TARGET_MFCRF)
return;
+ fputc (',', file);
+ /* FALLTHRU */
case 'R':
/* X is a CR register. Print the mask for `mtcrf'. */
Likewise.
Will do.
/* Return the string to output a conditional branch to LABEL, which is
- the operand number of the label, or -1 if the branch is really a
+ the operand template of the label, or NULL if the branch is really a
conditional return.
OP is the conditional expression. XEXP (OP, 0) is assumed to be a
This looks like it should be done separately also.
OK.
+bool
+valid_vle_sd4_field (rtx mem, enum machine_mode mode)
No comment before this function.
I'll fix that.
- (simple_return )])
+ (simple_return 1)])
Submitted separately it looks.
Sure.
libgcc/longlong.h
Gets sync'd with glibc's version sometimes.
Are you asking me to change something here?
Also have you thought have just adding a vle.md for all the needed
patterns and disabling the patterns in rs6000.md for VLE and not using
%^/%+/%- ? I think that would have been a cleaner implementation of
vle than adding support for it to the current patterns. Also does not
have the maintenance issue of always having to check if a new pattern
needs the %^/%+/%-.
The idea was discussed. A disadvantage is the code duplication it
would cause, which is also a maintenance headache. So we opted for the
current implementation. If the mainstream development causes a problem
for VLE it will be up to those interested in VLE to correct %^ etc.
--
Jim Lemke
Mentor Graphics / CodeSourcery
Orillia Ontario, +1-613-963-1073
Re: [PATCH] PowerPC VLE port
On Tue, 11 Sep 2012, David Edelsohn wrote: 2012-09-10 Maciej W. Rozycki ma...@codesourcery.com 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
Re: [PATCH] PowerPC VLE port
2012-09-10 Maciej W. Rozycki ma...@codesourcery.com 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
Re: [PATCH] PowerPC VLE port
On Tue, 11 Sep 2012, Segher Boessenkool wrote: 2012-09-10 Maciej W. Rozycki ma...@codesourcery.com 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));
Re: [PATCH] PowerPC VLE port
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
Re: [PATCH] PowerPC VLE port
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
Re: [PATCH] PowerPC VLE port
On Mon, Sep 10, 2012 at 5:28 PM, Maciej W. Rozycki ma...@codesourcery.com 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 ma...@codesourcery.com gcc/ * config/rs6000/rs6000.c (print_operand) 'c': Remove. * config/rs6000/spe.md: Remove a leftover comment. Okay. Thanks, David
Re: [PATCH] PowerPC VLE port
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
Re: [PATCH] PowerPC VLE port
On Thu, Sep 6, 2012 at 2:55 PM, James Lemke jwle...@codesourcery.com 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
Re: [PATCH] PowerPC VLE port
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
Re: [PATCH] PowerPC VLE port
On Thu, Sep 6, 2012 at 3:39 PM, Maciej W. Rozycki ma...@codesourcery.com 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
Re: [PATCH] PowerPC VLE port
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
Re: [PATCH] PowerPC VLE port
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 jos...@codesourcery.com
Re: [PATCH] PowerPC VLE port
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
Re: [PATCH] PowerPC VLE port
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
