diff mbox

[mips] Fix warning when using --with-synci

Message ID 1eecf735-7c65-4330-8941-0edc3ce91c1d@EXCHHUB01.MIPS.com
State New
Headers show

Commit Message

Steve Ellcey June 20, 2012, 9:15 p.m. UTC
This patch addresses the problem of building GCC for mips with the
'--with-synci' configure option.  If you do that and then compile a
program with GCC and specify an architecture that does not support synci
(such as the -mips32 option), GCC will issue a warning that synci
is not supported.  This results in many problems including an inability
to build a multilib version of GCC that includes -mips32.  This
patch changes the gcc driver to pass --msynci-if-supported to cc1
instead of -msynci so that cc1 will only turn on synci if it is
supported on the architecture being compiled for and will leave it off
(and not generate a warning) if it is not supported on that architecture.
If the user specifically uses -msynci, they still get the warning.

Tested on mips-linux-gnu, OK for checkin?

Steve Ellcey
sellcey@mips.com


2012-06-20  Steve Ellcey  <sellcey@mips.com>

	* config.gcc: Set with_synci to synci-if-supported instead if synci.
	* config/mips/mips.c (mips_option_override): Check
	TARGET_SYNCI_IF_SUPPORTED and update target_flags.
	* config/mips/mips.opt (msynci-if-supported): New.

Comments

Richard Sandiford June 21, 2012, 6:05 p.m. UTC | #1
"Steve Ellcey " <sellcey@mips.com> writes:
> This patch addresses the problem of building GCC for mips with the
> '--with-synci' configure option.  If you do that and then compile a
> program with GCC and specify an architecture that does not support synci
> (such as the -mips32 option), GCC will issue a warning that synci
> is not supported.  This results in many problems including an inability
> to build a multilib version of GCC that includes -mips32.  This
> patch changes the gcc driver to pass --msynci-if-supported to cc1
> instead of -msynci so that cc1 will only turn on synci if it is
> supported on the architecture being compiled for and will leave it off
> (and not generate a warning) if it is not supported on that architecture.
> If the user specifically uses -msynci, they still get the warning.

I think --with-synci should be equivalent to -msynci, to maintain
consistency.

This wouldn't affect targets with -mabi-only multilibs like
mips64-linux-gnu, so I assume you're trying to provide a stock
one-size-fits-all configuration like mips-sde-elf, but with synci
on by default for r2 and above.  If so, then maybe we should have
a configuration for that.

Is it mips-sde-elf you're interested in, or something else?  And do
you think builds both with and without --with-synci are useful for that
configuration?  (Instead of having multilibs for both, say.  The default
would be up to you.)

The reason I'm asking is that, if we're having to adjust certain
multilibs in a configuration in this way, it sounds to me like we're
either missing multilibs in that configuration, or have chosen the
wrong out-of-the-box defaults.

Richard
Steve Ellcey June 21, 2012, 9:29 p.m. UTC | #2
On Thu, 2012-06-21 at 19:05 +0100, Richard Sandiford wrote:
> "Steve Ellcey " <sellcey@mips.com> writes:
> > This patch addresses the problem of building GCC for mips with the
> > '--with-synci' configure option.  If you do that and then compile a
> > program with GCC and specify an architecture that does not support synci
> > (such as the -mips32 option), GCC will issue a warning that synci
> > is not supported.  This results in many problems including an inability
> > to build a multilib version of GCC that includes -mips32.  This
> > patch changes the gcc driver to pass --msynci-if-supported to cc1
> > instead of -msynci so that cc1 will only turn on synci if it is
> > supported on the architecture being compiled for and will leave it off
> > (and not generate a warning) if it is not supported on that architecture.
> > If the user specifically uses -msynci, they still get the warning.
> 
> I think --with-synci should be equivalent to -msynci, to maintain
> consistency.
> 
> This wouldn't affect targets with -mabi-only multilibs like
> mips64-linux-gnu, so I assume you're trying to provide a stock
> one-size-fits-all configuration like mips-sde-elf, but with synci
> on by default for r2 and above.  If so, then maybe we should have
> a configuration for that.

That is basically what I am trying to do, build a GCC cross compiler for
the mips-linux-gnu target that can support both ABI's/architectures that
have synci and ones that don't.

I haven't verified this but if I build a GCC for the mips64-linux-gnu
target and configure it with --with-synci and then use that compiler
with the -mabi=32 won't I run into this same problem?  I think I would
get a warning on those compilations and because of that the libstdc++
build will fail.  I will try that when I get a chance but I don't have a
mips64-linux-gnu GCC right now.

> Is it mips-sde-elf you're interested in, or something else?  And do
> you think builds both with and without --with-synci are useful for that
> configuration?  (Instead of having multilibs for both, say.  The default
> would be up to you.)

I was looking at the mips-linux-gnu target and trying to build a
multilib version of it with:

	MULTILIB_OPTIONS = EL/EB msoft-float mips32

It is the -mips32 that gives me the problem.  Even if I tied this to a
different target name I would still have the problem of not wanting a
warning message when compiling with -mips32 but wanting synci for the
other compilations.

> The reason I'm asking is that, if we're having to adjust certain
> multilibs in a configuration in this way, it sounds to me like we're
> either missing multilibs in that configuration, or have chosen the
> wrong out-of-the-box defaults.

I think the problem is that we don't have a good default behavior for
synci, which would be 'use it if you have it but don't give a warning if
you don't have it'  and that is what I am trying to create here.

Steve Ellcey
sellcey@mips.com
Richard Sandiford June 22, 2012, 9:24 a.m. UTC | #3
Steve Ellcey <sellcey@mips.com> writes:
> That is basically what I am trying to do, build a GCC cross compiler for
> the mips-linux-gnu target that can support both ABI's/architectures that
> have synci and ones that don't.
>
> I haven't verified this but if I build a GCC for the mips64-linux-gnu
> target and configure it with --with-synci and then use that compiler
> with the -mabi=32 won't I run into this same problem?  I think I would
> get a warning on those compilations and because of that the libstdc++
> build will fail.  I will try that when I get a chance but I don't have a
> mips64-linux-gnu GCC right now.

It ought to work.  mips64-linux-gnu itself defaults to MIPS I for 32-bit
and MIPS III for 64-bit, so --with-synci on its own wouldn't be much use.
But if you configure with --with-arch=mips64r2 --with-synci, say,
then -mabi=32 should build o32 for mips64r2.

>> Is it mips-sde-elf you're interested in, or something else?  And do
>> you think builds both with and without --with-synci are useful for that
>> configuration?  (Instead of having multilibs for both, say.  The default
>> would be up to you.)
>
> I was looking at the mips-linux-gnu target and trying to build a
> multilib version of it with:
>
> 	MULTILIB_OPTIONS = EL/EB msoft-float mips32
>
> It is the -mips32 that gives me the problem.  Even if I tied this to a
> different target name I would still have the problem of not wanting a
> warning message when compiling with -mips32 but wanting synci for the
> other compilations.

If you use a different target name, the specs for that target can
enforce whatever triplet-specific defaults you want.  See the
DRIVER_SELF_SPECS in vr.h for a particularly involved example.
(Yours shouldn't need to be as bad!)

How are you expecting to use this configuration?  Is the sysroot
that backs it going to have C libraries for all combinations?
I assume so, since otherwise you wouldn't be able to build
more than libgcc.

If so, we need to define what the OS library directories
are for -EB vs -EL, -mhard-float vs. -msoft-float, etc.
Are you planning to extend the IRIX lib/lib32/lib64 system,
or use the Debian/Ubuntu multiarch?

Richard
Steve Ellcey June 25, 2012, 5:02 p.m. UTC | #4
On Fri, 2012-06-22 at 10:24 +0100, Richard Sandiford wrote:

> If you use a different target name, the specs for that target can
> enforce whatever triplet-specific defaults you want.  See the
> DRIVER_SELF_SPECS in vr.h for a particularly involved example.
> (Yours shouldn't need to be as bad!)

I think that using DRIVER_SELF_SPECS would still require me to figure
out exactly what architectures support -msynci and which don't in order
to pass the right thing.  What I liked about -msynci-if-supported is
that it means there is only one check to determine if synci is
supported, the one in mips.h, and so I don't have to worry about two
different tests not being consistent.

> How are you expecting to use this configuration?  Is the sysroot
> that backs it going to have C libraries for all combinations?
> I assume so, since otherwise you wouldn't be able to build
> more than libgcc.

Yes, I would build a C library for each combination.

> If so, we need to define what the OS library directories
> are for -EB vs -EL, -mhard-float vs. -msoft-float, etc.
> Are you planning to extend the IRIX lib/lib32/lib64 system,
> or use the Debian/Ubuntu multiarch?
> 
> Richard

I was looking at lib/lib32/lib64.  Here is the full t-mti file I was
trying to build with the latest sources:

MULTILIB_OPTIONS = EL/EB msoft-float mips32
MULTILIB_DIRNAMES = el eb soft-float mips32
MULTILIB_MATCHES := EL=mel EB=meb
EXTRA_MULTILIB_PARTS = crtbegin.o crtend.o crtbeginS.o crtendS.o crtbeginT.o

and I also have a mti.h header file with:

#define MULTILIB_DEFAULTS { "EB" }
#define SYSROOT_SUFFIX_SPEC \
  "%{mel|EL:/el}%{msoft-float:/soft-float}%{mips32:/mips32}"

Steve Ellcey
sellcey@mips.com
Richard Sandiford June 25, 2012, 7 p.m. UTC | #5
Steve Ellcey <sellcey@mips.com> writes:
> On Fri, 2012-06-22 at 10:24 +0100, Richard Sandiford wrote:
>
>> If you use a different target name, the specs for that target can
>> enforce whatever triplet-specific defaults you want.  See the
>> DRIVER_SELF_SPECS in vr.h for a particularly involved example.
>> (Yours shouldn't need to be as bad!)
>
> I think that using DRIVER_SELF_SPECS would still require me to figure
> out exactly what architectures support -msynci and which don't in order
> to pass the right thing.

Not if you use MIPS_ISA_LEVEL_SPEC.  I think you'd want that anyway,
in order to pick the right multilib.  E.g. MIPS_ISA_LEVEL_SPEC is
the thing that would make -march=4kc pick the mips32 rather than
mips32r2 multilibs.

Since you have soft-float multilibs, you'd probably also want
MIPS_ARCH_FLOAT_SPEC.

Or to put it another way, MIPS_ISA_LEVEL_SPEC and MIPS_ARCH_FLOAT_SPEC
make the multilib options explicit on the command line.  You can then
use that information to add whatever options you want to be the default
for a given multilib.

> What I liked about -msynci-if-supported is that it means there is only
> one check to determine if synci is supported, the one in mips.h, and
> so I don't have to worry about two different tests not being
> consistent.

It opens up a can of worms though.  I think these --with-* options
should be consistent.  If we say that --with-synci should be overridden
by anything that is incompatible with it, then the same should apply
to --with-abi, --with-arch, etc.  (And the same should be true for
things like --with-mode on ARM.)

E.g. if you pass -mgp64 to a compiler that was configured with --with-arch,
then presumably we should ignore that --with-arch if it specifies a 32-bit
architecture, but continue to honour it if it specifies a 64-bit
architecture.  (I realise we have --with-arch-32 and --with-arch-64, but
the principle holds regardless.)  So say you have a target that is usually
mips64 by default, but that has 32-bit multilibs.  If you configure with
--with-arch=mips32r2 --with-synci, then pass -mgp64, what should happen?
I assume we'd revert back to the default of mips64 and then (after your
change) silently drop the -msynci too.  And that feels like too much
guesswork on our part.  Maybe the user really wanted mips64r2.

That's just one example.  The same thing would apply to --with-abi.
The relationships between these options are so complicated that
I think the user interface should be as simple as possible.
And IMO, that means that if the user explicitly configures with
--with-synci, then they should use -mno-synci if they want to
reverse that decision.

Packagers who want to provide both -msynci and -mno-synci multilibs
(with mips32 being one possible form of the latter) should IMO define
an appropriate configuration that doesn't rely on --with-synci.

>> If so, we need to define what the OS library directories
>> are for -EB vs -EL, -mhard-float vs. -msoft-float, etc.
>> Are you planning to extend the IRIX lib/lib32/lib64 system,
>> or use the Debian/Ubuntu multiarch?
>> 
>> Richard
>
> I was looking at lib/lib32/lib64.  Here is the full t-mti file I was
> trying to build with the latest sources:
>
> MULTILIB_OPTIONS = EL/EB msoft-float mips32
> MULTILIB_DIRNAMES = el eb soft-float mips32
> MULTILIB_MATCHES := EL=mel EB=meb
> EXTRA_MULTILIB_PARTS = crtbegin.o crtend.o crtbeginS.o crtendS.o crtbeginT.o
>
> and I also have a mti.h header file with:
>
> #define MULTILIB_DEFAULTS { "EB" }
> #define SYSROOT_SUFFIX_SPEC \
>   "%{mel|EL:/el}%{msoft-float:/soft-float}%{mips32:/mips32}"

Ah, OK, I get it now.  Thanks.

Richard
Steve Ellcey June 25, 2012, 9:05 p.m. UTC | #6
On Mon, 2012-06-25 at 20:00 +0100, Richard Sandiford wrote:

> Or to put it another way, MIPS_ISA_LEVEL_SPEC and MIPS_ARCH_FLOAT_SPEC
> make the multilib options explicit on the command line.  You can then
> use that information to add whatever options you want to be the default
> for a given multilib.

OK, I didn't really understand this setup before, but I think I get it
now and I see how I would use this to set -msynci.  I guess I would
want to create a new triple for a multilib linux mips target so I will
work on that approach and see how it goes.

Steve Ellcey
sellcey@mips.com
Richard Sandiford June 26, 2012, 6:51 p.m. UTC | #7
Steve Ellcey <sellcey@mips.com> writes:
> On Mon, 2012-06-25 at 20:00 +0100, Richard Sandiford wrote:
>> Or to put it another way, MIPS_ISA_LEVEL_SPEC and MIPS_ARCH_FLOAT_SPEC
>> make the multilib options explicit on the command line.  You can then
>> use that information to add whatever options you want to be the default
>> for a given multilib.
>
> OK, I didn't really understand this setup before, but I think I get it
> now and I see how I would use this to set -msynci.  I guess I would
> want to create a new triple for a multilib linux mips target so I will
> work on that approach and see how it goes.

In case the run-around over this patch has put you off: it'd be
great if you could submit the configuration once you're happy with it.
If MIPS think a target like this is needed then that's good enough for me.

Thanks,
Richard
Steve Ellcey June 26, 2012, 8:25 p.m. UTC | #8
On Tue, 2012-06-26 at 19:51 +0100, Richard Sandiford wrote:

> > OK, I didn't really understand this setup before, but I think I get it
> > now and I see how I would use this to set -msynci.  I guess I would
> > want to create a new triple for a multilib linux mips target so I will
> > work on that approach and see how it goes.
> 
> In case the run-around over this patch has put you off: it'd be
> great if you could submit the configuration once you're happy with it.
> If MIPS think a target like this is needed then that's good enough for me.
> 
> Thanks,
> Richard

Yes, I will submit it once I have something.  I am new to MIPS and still
trying to understand all the different variations and how they all
relate to each other.

Steve Ellcey
sellcey@mips.com
diff mbox

Patch

diff --git a/gcc/config.gcc b/gcc/config.gcc
index f2b0936..58ee3e9 100644
--- a/gcc/config.gcc
+++ b/gcc/config.gcc
@@ -3281,7 +3281,7 @@  case "${target}" in
 
 		case ${with_synci} in
 		yes)
-			with_synci=synci
+			with_synci=synci-if-supported
 			;;
 		"" | no)
 			# No is the default.
diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index 5bcb7a8..f17d39b 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -16172,6 +16172,9 @@  mips_option_override (void)
           : !TARGET_BRANCHLIKELY))
     sorry ("%qs requires branch-likely instructions", "-mfix-r10000");
 
+  if (TARGET_SYNCI_IF_SUPPORTED && !TARGET_SYNCI && ISA_HAS_SYNCI)
+    target_flags |= MASK_SYNCI;
+
   if (TARGET_SYNCI && !ISA_HAS_SYNCI)
     {
       warning (0, "the %qs architecture does not support the synci "
diff --git a/gcc/config/mips/mips.opt b/gcc/config/mips/mips.opt
index e3294a7..1dbce65 100644
--- a/gcc/config/mips/mips.opt
+++ b/gcc/config/mips/mips.opt
@@ -338,6 +338,9 @@  msynci
 Target Report Mask(SYNCI)
 Use synci instruction to invalidate i-cache
 
+msynci-if-supported
+Target Mask(SYNCI_IF_SUPPORTED) RejectNegative Undocumented
+
 mtune=
 Target RejectNegative Joined Var(mips_tune_option) ToLower Enum(mips_arch_opt_value)
 -mtune=PROCESSOR	Optimize the output for PROCESSOR