subject:"RE\: \[PATCH, MIPS\] Target flag and build option to disable indexed memory OPs."

RE: [PATCH, MIPS] Target flag and build option to disable indexed memory OPs.

2017-01-19 Thread Matthew Fortune

Hi Doug,

I've committed this on your behalf to get the testcases in and also
add the description of when this feature is required.  Thanks for the
patch.  Committed code inline below.

r244640

gcc/

PR target/78176
* config.gcc (supported_defaults): Add lxc1-sxc1.
(with_lxc1_sxc1): Add validation.
(all_defaults): Add lxc1-sxc1.
* config/mips/mips.opt (mlxc1-sxc1): New option.
* gcc/config/mips/mips.h (OPTION_DEFAULT_SPECS): Add a default for
mlxc1-sxc1.
(TARGET_CPU_CPP_BUILTINS): Add builtin_define for
__mips_no_lxc1_sxc1.
(ISA_HAS_LXC1_SXC1): Gate with mips_lxc1_sxc1.
* gcc/doc/invoke.texi (-mlxc1-sxc1): Document the new option.
* doc/install.texi (--with-lxc1-sxc1): Document the new option.

gcc/testsuite/

* gcc.target/mips/lxc1-sxc1-1.c: New file.
* gcc.target/mips/lxc1-sxc1-2.c: Likewise.
* gcc.target/mips/mips.exp (mips_option_groups): Add ghost option
HAS_LXC1.
(mips_option_groups): Add -m[no-]lxc1-sxc1.
(mips-dg-init): Detect default -mno-lxc1-sxc1.
(mips-dg-options): Handle HAS_LXC1 arch upgrade/downgrade.

Matthew

git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@244640 
138bc75d-0d04-0410-961f-82ee72b054a4
---
 gcc/ChangeLog   | 15 
 gcc/config.gcc  | 19 -
 gcc/config/mips/mips.h  |  8 +++-
 gcc/config/mips/mips.opt|  4 ++
 gcc/doc/install.texi| 19 +
 gcc/doc/invoke.texi |  6 +++
 gcc/testsuite/ChangeLog | 11 ++
 gcc/testsuite/gcc.target/mips/lxc1-sxc1-1.c | 60 +
 gcc/testsuite/gcc.target/mips/lxc1-sxc1-2.c | 60 +
 gcc/testsuite/gcc.target/mips/mips.exp  | 12 +-
 10 files changed, 209 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/mips/lxc1-sxc1-1.c
 create mode 100644 gcc/testsuite/gcc.target/mips/lxc1-sxc1-2.c

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 20b703f..f933e1ad 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,18 @@
+2017-01-19  Doug Gilmore  
+
+   PR target/78176
+   * config.gcc (supported_defaults): Add lxc1-sxc1.
+   (with_lxc1_sxc1): Add validation.
+   (all_defaults): Add lxc1-sxc1.
+   * config/mips/mips.opt (mlxc1-sxc1): New option.
+   * gcc/config/mips/mips.h (OPTION_DEFAULT_SPECS): Add a default for
+   mlxc1-sxc1.
+   (TARGET_CPU_CPP_BUILTINS): Add builtin_define for
+   __mips_no_lxc1_sxc1.
+   (ISA_HAS_LXC1_SXC1): Gate with mips_lxc1_sxc1.
+   * gcc/doc/invoke.texi (-mlxc1-sxc1): Document the new option.
+   * doc/install.texi (--with-lxc1-sxc1): Document the new option.
+
 2017-01-19  Richard Biener  
 
PR tree-optimization/72488
diff --git a/gcc/config.gcc b/gcc/config.gcc
index 90308cd..dd8c08c 100644
--- a/gcc/config.gcc
+++ b/gcc/config.gcc
@@ -3940,7 +3940,7 @@ case "${target}" in
;;
 
mips*-*-*)
-   supported_defaults="abi arch arch_32 arch_64 float fpu nan 
fp_32 odd_spreg_32 tune tune_32 tune_64 divide llsc mips-plt synci"
+   supported_defaults="abi arch arch_32 arch_64 float fpu nan 
fp_32 odd_spreg_32 tune tune_32 tune_64 divide llsc mips-plt synci lxc1-sxc1"
 
case ${with_float} in
"" | soft | hard)
@@ -4063,6 +4063,21 @@ case "${target}" in
exit 1
;;
esac
+
+   case ${with_lxc1_sxc1} in
+   yes)
+   with_lxc1_sxc1=lxc1-sxc1
+   ;;
+   no)
+   with_lxc1_sxc1=no-lxc1-sxc1
+   ;;
+   "")
+   ;;
+   *)
+   echo "Unknown lxc1-sxc1 type used in --with-lxc1-sxc1" 
1>&2
+   exit 1
+   ;;
+   esac
;;
 
nds32*-*-*)
@@ -4496,7 +4511,7 @@ case ${target} in
 esac
 
 t=
-all_defaults="abi cpu cpu_32 cpu_64 arch arch_32 arch_64 tune tune_32 tune_64 
schedule float mode fpu nan fp_32 odd_spreg_32 divide llsc mips-plt synci tls"
+all_defaults="abi cpu cpu_32 cpu_64 arch arch_32 arch_64 tune tune_32 tune_64 
schedule float mode fpu nan fp_32 odd_spreg_32 divide llsc mips-plt synci tls 
lxc1-sxc1"
 for option in $all_defaults
 do
eval "val=\$with_"`echo $option | sed s/-/_/g`
diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h
index fbd7011..4205589 100644
--- a/gcc/config/mips/mips.h
+++ b/gcc/config/mips/mips.h
@@ -637,6 +637,8 @@ struct mips_cpu_info {
\
   if (TARGET_CACHE_BUILTIN)
\

Re: [PATCH, MIPS] Target flag and build option to disable indexed memory OPs.

2017-01-18 Thread Doug Gilmore

On 01/17/2017 05:41 AM, Moore, Catherine wrote:
> 
> 
>> -Original Message-
>> From: Matthew Fortune [mailto:matthew.fort...@imgtec.com]
>> Sent: Tuesday, January 17, 2017 4:35 AM
>>
...
>> Thanks for the comments.
>>
>> Having thought further I agree we can safely ignore DSP indexed load
>> and micromips LWXS on
>> the basis that DSP code will not run on a MIPS64 processor anyway (at
>> least none that I
>> know of) so the issue cannot occur and similarly for microMIPS, there
>> are no 64-bit cores.
>>
>> Restricting to just LWXC1/SWXC1/LDXC1/SDXC1 is therefore fine but
>> we should reflect
>> that in option names then.
>>
>> --with-lxc1-sxc1 --without-lxc1-sxc1
>> -mlxc1-sxc1
>>
>> These names reflect the internal macro that controls availability of
>> these instructions.
>>
>> Macro name: __mips_no_lxc1_sxc1
>> Defined when !ISA_HAS_LXC1_SXC1 so would be present even when
>> targeting a core that
>> doesn't have the instructions anyway.
>>
>> Any refinements to this Catherine?
>>
> No.  This plan looks good.
> 
Hi Everyone,

I updated the patch accordingly.  OK to commit?

Thanks,

Doug

>From 5aa6e7b837a281651ac1c6c58291c96d6ff25c53 Mon Sep 17 00:00:00 2001
From: Doug Gilmore 
Date: Wed, 11 Jan 2017 16:49:27 -0800
Subject: [PATCH] [MIPS] PR target/78176 add -mlxc1-sxc1.

	PR target/78176
	* config.gcc (supported_defaults): Add lxc1-sxc1.
	(with_lxc1_sxc1): Add validation.
	(all_defaults): Add lxc1-sxc1.
	* config/mips/mips.opt (mlxc1-sxc1): New option.
	* gcc/config/mips/mips.h (OPTION_DEFAULT_SPECS): Add a default for
	mlxc1-sxc1.
	(TARGET_CPU_CPP_BUILTINS) Add builtin_define for
	__mips_no_lxc1_sxc1.
	ISA_HAS_LXC1_SXC1 gate with mips_lxc1_sxc1.
	* gcc/doc/invoke.texi (-mlxc1-sxc1): Document the new option.
	* doc/install.texi (--with-lxc1-sxc1): Document the new option.
---
 gcc/config.gcc   | 19 +--
 gcc/config/mips/mips.h   |  8 ++--
 gcc/config/mips/mips.opt |  4 
 gcc/doc/install.texi |  8 
 gcc/doc/invoke.texi  |  6 ++
 5 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/gcc/config.gcc b/gcc/config.gcc
index 7c27546..913e5c2 100644
--- a/gcc/config.gcc
+++ b/gcc/config.gcc
@@ -3940,7 +3940,7 @@ case "${target}" in
 		;;
 
 	mips*-*-*)
-		supported_defaults="abi arch arch_32 arch_64 float fpu nan fp_32 odd_spreg_32 tune tune_32 tune_64 divide llsc mips-plt synci"
+		supported_defaults="abi arch arch_32 arch_64 float fpu nan fp_32 odd_spreg_32 tune tune_32 tune_64 divide llsc mips-plt synci lxc1-sxc1"
 
 		case ${with_float} in
 		"" | soft | hard)
@@ -4063,6 +4063,21 @@ case "${target}" in
 			exit 1
 			;;
 		esac
+
+		case ${with_lxc1_sxc1} in
+		yes)
+			with_lxc1_sxc1=lxc1-sxc1
+			;;
+		no)
+			with_lxc1_sxc1=no-lxc1-sxc1
+			;;
+		"")
+			;;
+		*)
+			echo "Unknown lxc1-sxc1 type used in --with-lxc1-sxc1" 1>&2
+			exit 1
+			;;
+		esac
 		;;
 
 	nds32*-*-*)
@@ -4496,7 +4511,7 @@ case ${target} in
 esac
 
 t=
-all_defaults="abi cpu cpu_32 cpu_64 arch arch_32 arch_64 tune tune_32 tune_64 schedule float mode fpu nan fp_32 odd_spreg_32 divide llsc mips-plt synci tls"
+all_defaults="abi cpu cpu_32 cpu_64 arch arch_32 arch_64 tune tune_32 tune_64 schedule float mode fpu nan fp_32 odd_spreg_32 divide llsc mips-plt synci tls lxc1-sxc1"
 for option in $all_defaults
 do
 	eval "val=\$with_"`echo $option | sed s/-/_/g`
diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h
index f91b43d..6d9a7aa 100644
--- a/gcc/config/mips/mips.h
+++ b/gcc/config/mips/mips.h
@@ -637,6 +637,8 @@ struct mips_cpu_info {
 	\
   if (TARGET_CACHE_BUILTIN)		\
 	builtin_define ("__GCC_HAVE_BUILTIN_MIPS_CACHE");		\
+  if (!ISA_HAS_LXC1_SXC1)		\
+	builtin_define ("__mips_no_lxc1_sxc1");\
 }	\
   while (0)
 
@@ -866,7 +868,8 @@ struct mips_cpu_info {
   {"divide", "%{!mdivide-traps:%{!mdivide-breaks:-mdivide-%(VALUE)}}" }, \
   {"llsc", "%{!mllsc:%{!mno-llsc:-m%(VALUE)}}" }, \
   {"mips-plt", "%{!mplt:%{!mno-plt:-m%(VALUE)}}" }, \
-  {"synci", "%{!msynci:%{!mno-synci:-m%(VALUE)}}" }
+  {"synci", "%{!msynci:%{!mno-synci:-m%(VALUE)}}" },			\
+  {"lxc1-sxc1", "%{!mlxc1-sxc1:%{!mno-lxc1-sxc1:-m%(VALUE)}}" } \
 
 /* A spec that infers the:
-mnan=2008 setting from a -mips argument,
@@ -1030,7 +1033,8 @@ struct mips_cpu_info {
 
 /* ISA has floating-point indexed load and store instructions
(LWXC1, LDXC1, SWXC1 and SDXC1).  */
-#define ISA_HAS_LXC1_SXC1	ISA_HAS_FP4
+#define ISA_HAS_LXC1_SXC1	(ISA_HAS_FP4\
+ && mips_lxc1_sxc1)
 
 /* ISA has paired-single instructions.  */
 #define ISA_HAS_PAIRED_SINGLE	((ISA_MIPS64\
diff --git a/gcc/config/mips/mips.opt b/gcc/config/mips/mips.opt
index 2559649..347f552 100644
--- a/gcc/config/mips/mips.opt
+++ b/gcc/config/mips/mips.opt
@@ -388,6 +388,10 @@ mlra
 Target Report Var(mips_lra_flag) Init(1) Save
 Use LRA instead of reload.
 
+mlxc1-sxc1
+Target Report Var(mips_lxc1_sxc1) Init(1)
+Use lxc1/sxc1 instructions where applicable.
+

Re: [PATCH, MIPS] Target flag and build option to disable indexed memory OPs.

2017-01-17 Thread Doug Gilmore

On 01/17/2017 05:41 AM, Moore, Catherine wrote:
> 
>
>> ...
>> Having thought further I agree we can safely ignore DSP indexed load
>> and micromips LWXS on
>> the basis that DSP code will not run on a MIPS64 processor anyway (at
>> least none that I
>> know of) so the issue cannot occur and similarly for microMIPS, there
>> are no 64-bit cores.
>>
>> Restricting to just LWXC1/SWXC1/LDXC1/SDXC1 is therefore fine but
>> we should reflect
>> that in option names then.
>>
>> --with-lxc1-sxc1 --without-lxc1-sxc1
>> -mlxc1-sxc1
>>
>> These names reflect the internal macro that controls availability of
>> these instructions.
>>
>> Macro name: __mips_no_lxc1_sxc1
>> Defined when !ISA_HAS_LXC1_SXC1 so would be present even when
>> targeting a core that
>> doesn't have the instructions anyway.
>>
>> Any refinements to this Catherine?
>>
> No.  This plan looks good.
> 
Sounds good, I'll update the patch accordingly.

BTW, if we did guard all of the indexed memory OPs with a flag
there would be ~150 tests to clean up when configuring with indexed
memory OPs disabled.  When I tested with indexed memory OPs disabled
with the original patch, there were no additional regressions.

Also I'll be updating the bug report with my current take on what went
wrong with r216501.

Thanks,

Doug

RE: [PATCH, MIPS] Target flag and build option to disable indexed memory OPs.

2017-01-17 Thread Moore, Catherine



> -Original Message-
> From: Matthew Fortune [mailto:matthew.fort...@imgtec.com]
> Sent: Tuesday, January 17, 2017 4:35 AM
> To: Moore, Catherine <catherine_mo...@mentor.com>; Doug
> Gilmore <doug.gilm...@imgtec.com>; gcc-patches@gcc.gnu.org
> Subject: RE: [PATCH, MIPS] Target flag and build option to disable
> indexed memory OPs.
> 
> Moore, Catherine <catherine_mo...@mentor.com> writes:
> > > -Original Message-
> > > From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
> > > ow...@gcc.gnu.org] On Behalf Of Matthew Fortune
> > > Sent: Monday, January 16, 2017 11:25 AM
> > > To: Doug Gilmore <doug.gilm...@imgtec.com>; gcc-
> > > patc...@gcc.gnu.org
> > > Cc: Moore, Catherine <catherine_mo...@mentor.com>
> > > Subject: RE: [PATCH, MIPS] Target flag and build option to disable
> > > indexed memory OPs.
> > >
> > > Doug Gilmore <doug.gilm...@imgtec.com>
> > > > I recently bisected PR78176 to problems introduced with r21650.
> > > >
> > > > Given the short time until the release, we would like to provide a
> > > > target flag and build option to avoid the bug until we are able to
> > > > resolve the problem with the commit.  Note that as Matthew
> Fortune
> > > has
> > > > mentioned in the PR:
> > > >
> > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78176#c5
> > > >
> > > > the problem could also be addressed by updates to the Linux
> kernel
> > > since
> > > > the problem is only exposed by running MIPS 32-bit binaries on
> 64-
> > > bit
> > > > kernels.
> > > >
> > > > Bootstrapped on X86_64, regression tested on X86_64 and MIPS.
> > > >
> > > > OK to commit?
> > >
> > > Given this is a generic reference to indexed load/store and the
> issue
> > > could
> > > affect any indexed operation then I think it needs to include all of
> the
> > > following as well:
> > >
> > > /* ISA has lwxs instruction (load w/scaled index address.  */
> > > #define ISA_HAS_LWXS((TARGET_SMARTMIPS ||
> > > TARGET_MICROMIPS) \
> > >  && !TARGET_MIPS16)
> > >
> > > /* ISA has lbx, lbux, lhx, lhx, lhux, lwx, lwux, or ldx instruction. */
> > > #define ISA_HAS_LBX (TARGET_OCTEON2)
> > > #define ISA_HAS_LBUX(ISA_HAS_DSP || TARGET_OCTEON2)
> > > #define ISA_HAS_LHX (ISA_HAS_DSP || TARGET_OCTEON2)
> > > #define ISA_HAS_LHUX(TARGET_OCTEON2)
> > > #define ISA_HAS_LWX (ISA_HAS_DSP || TARGET_OCTEON2)
> > > #define ISA_HAS_LWUX(TARGET_OCTEON2 &&
> TARGET_64BIT)
> > > #define ISA_HAS_LDX ((ISA_HAS_DSP || TARGET_OCTEON2)
> \
> > >  && TARGET_64BIT)
> > >
> > > The DSP LBUX/LHX/LWX/LDX intrinsics will also need a new AVAIL
> > > predicate
> > > to disable them. The snag is that some DSP code will fail to compile
> if it
> > > uses the DSP load intrinsics directly.
> > >
> > > I see no way of avoiding that. Therefore, distributions that use
> > > --without-indexed-load-store will have to cope with some potential
> > > DSP
> > > fallout if they enable DSP at all.
> > >
> > > @Catherine: I'd like your input here if possible as I advocated this
> > > approach, comments on option names welcome too.  I quite like
> the
> > > verbose
> > > name.
> >
> > Okay, based on my reading of the comments in the bug report, you
> are proposing this option
> > as a workaround to a kernel deficiency.  I don't see any agreement
> that this is actually a
> > compiler bug.
> > Do we really need to include the DSP instrinsics as well?   Do you
> think that many
> > distributions actually enable DSP?
> >
> > The option name itself is acceptable to me.  I'd like to see
> documentation that explains
> > when this problem is exposed.  I'd like to limit the fix to LWXS and I'd
> like to see the
> > testcase from the bug report added to the testsuite.
> > I also agree that the preprocessor macro is a good idea (even if we
> decide to forgo the
> > DSP portion of the patch).
> 
> Thanks for the comments.
> 
> Having thought further I agree we can safely ignore DSP indexed load
> and micromips LWXS on
> the basis that DSP code will not run on a MIPS64 processor anyway (at
> least none that I
> know of) so the issue cannot occur and similarly for microMIPS, there
> are no 64-bit cores.
> 
> Restricting to just LWXC1/SWXC1/LDXC1/SDXC1 is therefore fine but
> we should reflect
> that in option names then.
> 
> --with-lxc1-sxc1 --without-lxc1-sxc1
> -mlxc1-sxc1
> 
> These names reflect the internal macro that controls availability of
> these instructions.
> 
> Macro name: __mips_no_lxc1_sxc1
> Defined when !ISA_HAS_LXC1_SXC1 so would be present even when
> targeting a core that
> doesn't have the instructions anyway.
> 
> Any refinements to this Catherine?
> 
No.  This plan looks good.

RE: [PATCH, MIPS] Target flag and build option to disable indexed memory OPs.

2017-01-17 Thread Matthew Fortune

Moore, Catherine <catherine_mo...@mentor.com> writes:
> > -Original Message-
> > From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
> > ow...@gcc.gnu.org] On Behalf Of Matthew Fortune
> > Sent: Monday, January 16, 2017 11:25 AM
> > To: Doug Gilmore <doug.gilm...@imgtec.com>; gcc-
> > patc...@gcc.gnu.org
> > Cc: Moore, Catherine <catherine_mo...@mentor.com>
> > Subject: RE: [PATCH, MIPS] Target flag and build option to disable
> > indexed memory OPs.
> >
> > Doug Gilmore <doug.gilm...@imgtec.com>
> > > I recently bisected PR78176 to problems introduced with r21650.
> > >
> > > Given the short time until the release, we would like to provide a
> > > target flag and build option to avoid the bug until we are able to
> > > resolve the problem with the commit.  Note that as Matthew Fortune
> > has
> > > mentioned in the PR:
> > >
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78176#c5
> > >
> > > the problem could also be addressed by updates to the Linux kernel
> > since
> > > the problem is only exposed by running MIPS 32-bit binaries on 64-
> > bit
> > > kernels.
> > >
> > > Bootstrapped on X86_64, regression tested on X86_64 and MIPS.
> > >
> > > OK to commit?
> >
> > Given this is a generic reference to indexed load/store and the issue
> > could
> > affect any indexed operation then I think it needs to include all of the
> > following as well:
> >
> > /* ISA has lwxs instruction (load w/scaled index address.  */
> > #define ISA_HAS_LWXS((TARGET_SMARTMIPS ||
> > TARGET_MICROMIPS) \
> >  && !TARGET_MIPS16)
> >
> > /* ISA has lbx, lbux, lhx, lhx, lhux, lwx, lwux, or ldx instruction. */
> > #define ISA_HAS_LBX (TARGET_OCTEON2)
> > #define ISA_HAS_LBUX(ISA_HAS_DSP || TARGET_OCTEON2)
> > #define ISA_HAS_LHX (ISA_HAS_DSP || TARGET_OCTEON2)
> > #define ISA_HAS_LHUX(TARGET_OCTEON2)
> > #define ISA_HAS_LWX (ISA_HAS_DSP || TARGET_OCTEON2)
> > #define ISA_HAS_LWUX(TARGET_OCTEON2 && TARGET_64BIT)
> > #define ISA_HAS_LDX ((ISA_HAS_DSP || TARGET_OCTEON2) \
> >  && TARGET_64BIT)
> >
> > The DSP LBUX/LHX/LWX/LDX intrinsics will also need a new AVAIL
> > predicate
> > to disable them. The snag is that some DSP code will fail to compile if it
> > uses the DSP load intrinsics directly.
> >
> > I see no way of avoiding that. Therefore, distributions that use
> > --without-indexed-load-store will have to cope with some potential
> > DSP
> > fallout if they enable DSP at all.
> >
> > @Catherine: I'd like your input here if possible as I advocated this
> > approach, comments on option names welcome too.  I quite like the
> > verbose
> > name.
> 
> Okay, based on my reading of the comments in the bug report, you are 
> proposing this option
> as a workaround to a kernel deficiency.  I don't see any agreement that this 
> is actually a
> compiler bug.
> Do we really need to include the DSP instrinsics as well?   Do you think that 
> many
> distributions actually enable DSP?
> 
> The option name itself is acceptable to me.  I'd like to see documentation 
> that explains
> when this problem is exposed.  I'd like to limit the fix to LWXS and I'd like 
> to see the
> testcase from the bug report added to the testsuite.
> I also agree that the preprocessor macro is a good idea (even if we decide to 
> forgo the
> DSP portion of the patch).

Thanks for the comments.

Having thought further I agree we can safely ignore DSP indexed load and 
micromips LWXS on
the basis that DSP code will not run on a MIPS64 processor anyway (at least 
none that I
know of) so the issue cannot occur and similarly for microMIPS, there are no 
64-bit cores.

Restricting to just LWXC1/SWXC1/LDXC1/SDXC1 is therefore fine but we should 
reflect
that in option names then.

--with-lxc1-sxc1 --without-lxc1-sxc1
-mlxc1-sxc1

These names reflect the internal macro that controls availability of these 
instructions.

Macro name: __mips_no_lxc1_sxc1
Defined when !ISA_HAS_LXC1_SXC1 so would be present even when targeting a core 
that
doesn't have the instructions anyway.

Any refinements to this Catherine?

Thanks,
Matthew

RE: [PATCH, MIPS] Target flag and build option to disable indexed memory OPs.

2017-01-16 Thread Moore, Catherine



> -Original Message-
> From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
> ow...@gcc.gnu.org] On Behalf Of Matthew Fortune
> Sent: Monday, January 16, 2017 11:25 AM
> To: Doug Gilmore <doug.gilm...@imgtec.com>; gcc-
> patc...@gcc.gnu.org
> Cc: Moore, Catherine <catherine_mo...@mentor.com>
> Subject: RE: [PATCH, MIPS] Target flag and build option to disable
> indexed memory OPs.
> 
> Doug Gilmore <doug.gilm...@imgtec.com>
> > I recently bisected PR78176 to problems introduced with r21650.
> >
> > Given the short time until the release, we would like to provide a
> > target flag and build option to avoid the bug until we are able to
> > resolve the problem with the commit.  Note that as Matthew Fortune
> has
> > mentioned in the PR:
> >
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78176#c5
> >
> > the problem could also be addressed by updates to the Linux kernel
> since
> > the problem is only exposed by running MIPS 32-bit binaries on 64-
> bit
> > kernels.
> >
> > Bootstrapped on X86_64, regression tested on X86_64 and MIPS.
> >
> > OK to commit?
> 
> Given this is a generic reference to indexed load/store and the issue
> could
> affect any indexed operation then I think it needs to include all of the
> following as well:
> 
> /* ISA has lwxs instruction (load w/scaled index address.  */
> #define ISA_HAS_LWXS((TARGET_SMARTMIPS ||
> TARGET_MICROMIPS) \
>  && !TARGET_MIPS16)
> 
> /* ISA has lbx, lbux, lhx, lhx, lhux, lwx, lwux, or ldx instruction. */
> #define ISA_HAS_LBX (TARGET_OCTEON2)
> #define ISA_HAS_LBUX(ISA_HAS_DSP || TARGET_OCTEON2)
> #define ISA_HAS_LHX (ISA_HAS_DSP || TARGET_OCTEON2)
> #define ISA_HAS_LHUX(TARGET_OCTEON2)
> #define ISA_HAS_LWX (ISA_HAS_DSP || TARGET_OCTEON2)
> #define ISA_HAS_LWUX(TARGET_OCTEON2 && TARGET_64BIT)
> #define ISA_HAS_LDX ((ISA_HAS_DSP || TARGET_OCTEON2) \
>  && TARGET_64BIT)
> 
> The DSP LBUX/LHX/LWX/LDX intrinsics will also need a new AVAIL
> predicate
> to disable them. The snag is that some DSP code will fail to compile if it
> uses the DSP load intrinsics directly.
> 
> I see no way of avoiding that. Therefore, distributions that use
> --without-indexed-load-store will have to cope with some potential
> DSP
> fallout if they enable DSP at all.
> 
> @Catherine: I'd like your input here if possible as I advocated this
> approach, comments on option names welcome too.  I quite like the
> verbose
> name.

Okay, based on my reading of the comments in the bug report, you are proposing 
this option as a workaround to a kernel deficiency.  I don't see any agreement 
that this is actually a compiler bug.
Do we really need to include the DSP instrinsics as well?   Do you think that 
many distributions actually enable DSP?  

The option name itself is acceptable to me.  I'd like to see documentation that 
explains when this problem is exposed.  I'd like to limit the fix to LWXS and 
I'd like to see the testcase from the bug report added to the testsuite.
I also agree that the preprocessor macro is a good idea (even if we decide to 
forgo the DSP portion of the patch).

Catherine

> 
> @Doug: Have you tried running the testsuite with the configure option
> --without-indexed-load-store? There may be tests that need adjusting
> where they
> test indexed load/store. We probably need a pre-processor macro
> to detect if the option is enabled/disabled so that DSP code can be
> predicated
> on indexed load being available.
> 
> Thanks,
> Matthew

RE: [PATCH, MIPS] Target flag and build option to disable indexed memory OPs.

2017-01-16 Thread Matthew Fortune

Doug Gilmore 
> I recently bisected PR78176 to problems introduced with r21650.
> 
> Given the short time until the release, we would like to provide a
> target flag and build option to avoid the bug until we are able to
> resolve the problem with the commit.  Note that as Matthew Fortune has
> mentioned in the PR:
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78176#c5
> 
> the problem could also be addressed by updates to the Linux kernel since
> the problem is only exposed by running MIPS 32-bit binaries on 64-bit
> kernels.
> 
> Bootstrapped on X86_64, regression tested on X86_64 and MIPS.
> 
> OK to commit?

Given this is a generic reference to indexed load/store and the issue could
affect any indexed operation then I think it needs to include all of the
following as well:

/* ISA has lwxs instruction (load w/scaled index address.  */
#define ISA_HAS_LWXS((TARGET_SMARTMIPS || TARGET_MICROMIPS) \
 && !TARGET_MIPS16)

/* ISA has lbx, lbux, lhx, lhx, lhux, lwx, lwux, or ldx instruction. */
#define ISA_HAS_LBX (TARGET_OCTEON2)
#define ISA_HAS_LBUX(ISA_HAS_DSP || TARGET_OCTEON2)
#define ISA_HAS_LHX (ISA_HAS_DSP || TARGET_OCTEON2)
#define ISA_HAS_LHUX(TARGET_OCTEON2)
#define ISA_HAS_LWX (ISA_HAS_DSP || TARGET_OCTEON2)
#define ISA_HAS_LWUX(TARGET_OCTEON2 && TARGET_64BIT)
#define ISA_HAS_LDX ((ISA_HAS_DSP || TARGET_OCTEON2) \
 && TARGET_64BIT)

The DSP LBUX/LHX/LWX/LDX intrinsics will also need a new AVAIL predicate
to disable them. The snag is that some DSP code will fail to compile if it
uses the DSP load intrinsics directly.

I see no way of avoiding that. Therefore, distributions that use
--without-indexed-load-store will have to cope with some potential DSP
fallout if they enable DSP at all.

@Catherine: I'd like your input here if possible as I advocated this
approach, comments on option names welcome too.  I quite like the verbose
name.

@Doug: Have you tried running the testsuite with the configure option
--without-indexed-load-store? There may be tests that need adjusting where they
test indexed load/store. We probably need a pre-processor macro
to detect if the option is enabled/disabled so that DSP code can be predicated
on indexed load being available.

Thanks,
Matthew

RE: [PATCH, MIPS] Target flag and build option to disable indexed memory OPs.

Re: [PATCH, MIPS] Target flag and build option to disable indexed memory OPs.

Re: [PATCH, MIPS] Target flag and build option to disable indexed memory OPs.

RE: [PATCH, MIPS] Target flag and build option to disable indexed memory OPs.

RE: [PATCH, MIPS] Target flag and build option to disable indexed memory OPs.

RE: [PATCH, MIPS] Target flag and build option to disable indexed memory OPs.

RE: [PATCH, MIPS] Target flag and build option to disable indexed memory OPs.

7 matches

Site Navigation

Mail list logo

Footer information