, V7, #5 of 7, Add more effective targets for the 'future' system to target-supports.
diff mbox series

Message ID 20191114225650.GE7528@ibm-toto.the-meissners.org
State New
Headers show
Series
  • , V7, #5 of 7, Add more effective targets for the 'future' system to target-supports.
Related show

Commit Message

Michael Meissner Nov. 14, 2019, 10:56 p.m. UTC
This patch adds more effective targets to the target-supports.exp in the
testsuite.  I tried to break it down to whether prefixed instructions are
allowed, whether the target is generating 64-bit code with prefixed
instructions, and if -mpcrel support is available.  I also enabled 'future'
testing on the actual hardware (or simulator).

The tests in V8 will use some of these capabilities.

I have run the test suite on a little endian power8 system with no degradation.
Can I check this into the FSF trunk?

2019-11-14   Michael Meissner  <meissner@linux.ibm.com>

	* lib/target-supports.exp
	(check_effective_target_powerpc_future_ok): Do not require 64-bit
	or Linux support before doing the test.  Use a 32-bit constant in
	PLI.
	(check_effective_target_powerpc_prefixed_addr_ok): New effective
	target test to see if prefixed memory instructions are supported.
	(check_effective_target_powerpc_pcrel_ok): New effective target
	test to test whether PC-relative addressing is supported.
	(is-effective-target): Add test for the PowerPC 'future' hardware
	support.

Comments

Segher Boessenkool Nov. 23, 2019, 2:11 a.m. UTC | #1
On Thu, Nov 14, 2019 at 05:56:50PM -0500, Michael Meissner wrote:
> 	* lib/target-supports.exp
> 	(check_effective_target_powerpc_future_ok): Do not require 64-bit
> 	or Linux support before doing the test.  Use a 32-bit constant in
> 	PLI.

You changed from 0x12345 to 0x1234, instead -- why?

> -# Return 1 if this is a PowerPC target supporting -mfuture.
> -# Limit this to 64-bit linux systems for now until other
> -# targets support FUTURE.
> +# Return 1 if this is a PowerPC target supporting -mcpu=future.

"Return 1 if the assembler supports Future instructions."

Please make it explicit that this isn't about the compiler.

>  proc check_effective_target_powerpc_future_ok { } {
> -    if { ([istarget powerpc64*-*-linux*]) } {
> +    if { ([istarget powerpc*-*-*]) } {
>  	return [check_no_compiler_messages powerpc_future_ok object {
>  	    int main (void) {
>  		long e;
> -		asm ("pli %0,%1" : "=r" (e) : "n" (0x12345));
> +		asm ("pli %0,%1" : "=r" (e) : "n" (0x1234));
>  		return e;
>  	    }
>  	} "-mfuture"]

You are still passing -mfuture.


> +# Return 1 if this is a PowerPC target supporting -mcpu=future.  The compiler
> +# must support large numeric prefixed addresses by default when -mfuture is
> +# used.  We test loading up a large constant to verify that the full 34-bit
> +# offset for prefixed instructions is supported and we check for a prefixed
> +# load as well.

The comment says one thing.

-mfuture isn't a compiler option...  Well it still is, but that should
be removed.

The actual test uses only 30 bits (and a positive number).  Which is fine,
but the comment is misleading then: the code doesn't test "if the full
34-bit offset is supported".

I don't understand why we test both pli and pld.

> +proc check_effective_target_powerpc_prefixed_addr_ok { } {

The name says another.

> +    if { ([istarget powerpc*-*-*]) } {

This part has no function?  Are there any testcases that test for our
prefixed insns that are compiler for non-powerpc?

If we want this at all, this test shouldn't be nested, just should be
an early-out.

> +	return [check_no_compiler_messages powerpc_prefixed_addr_ok object {
> +	    int main (void) {
> +		extern long l[];
> +		long e, e2;
> +		asm ("pli %0,%1" : "=r" (e) : "n" (0x12345678));
> +		asm ("pld %0,0x12345678(%1)" : "=r" (e2) : "r" (& l[0]));

(should be "b" for the last constraint; and "&l[0]" is usually written
just "l").

> +		return e - e2;
> +	    }
> +	} "-mfuture"]

And the code tests two things.

-mcpu=future, instead?

Does this need to be separate from check_effective_target_powerpc_future_ok
at all?

> +# Return 1 if this is a PowerPC target supporting -mfuture.  The compiler must

That is the third selector claiming to test the same thing ("target supports
-mfuture").

> +# support PC-relative addressing when -mcpu=future is used to pass this test.
> +
> +proc check_effective_target_powerpc_pcrel_ok { } {
> +    if { ([istarget powerpc*-*-*]) } {
> +	return [check_no_compiler_messages powerpc_pcrel_ok object {
> +	      int main (void) {
> +		  static int s __attribute__((__used__));
> +		  int e;
> +		  asm ("plwa %0,s@pcrel(0),1" : "=r" (e));
> +		  return e;
> +	      }
> +	  } "-mfuture"]
> +      } else {
> +	  return 0
> +      }
> +}

So every assembler will support either all three of these, or none.
Can you simplify this please?


Segher
Michael Meissner Dec. 3, 2019, 6:11 p.m. UTC | #2
On Fri, Nov 22, 2019 at 08:11:16PM -0600, Segher Boessenkool wrote:
> On Thu, Nov 14, 2019 at 05:56:50PM -0500, Michael Meissner wrote:
> > 	* lib/target-supports.exp
> > 	(check_effective_target_powerpc_future_ok): Do not require 64-bit
> > 	or Linux support before doing the test.  Use a 32-bit constant in
> > 	PLI.
> 
> You changed from 0x12345 to 0x1234, instead -- why?

The last time I did the patch there was discussion about 32-bit support.  I
just rewrote the patch to accomidate possibly using -mcpu=future on a 32-bit
platform.  In particular, AIX.  So I rewrote the basic test so that in theory
it would run on a 32-bit platform.

> > -# Return 1 if this is a PowerPC target supporting -mfuture.
> > -# Limit this to 64-bit linux systems for now until other
> > -# targets support FUTURE.
> > +# Return 1 if this is a PowerPC target supporting -mcpu=future.
> 
> "Return 1 if the assembler supports Future instructions."
> 
> Please make it explicit that this isn't about the compiler.

Ok.

> >  proc check_effective_target_powerpc_future_ok { } {
> > -    if { ([istarget powerpc64*-*-linux*]) } {
> > +    if { ([istarget powerpc*-*-*]) } {
> >  	return [check_no_compiler_messages powerpc_future_ok object {
> >  	    int main (void) {
> >  		long e;
> > -		asm ("pli %0,%1" : "=r" (e) : "n" (0x12345));
> > +		asm ("pli %0,%1" : "=r" (e) : "n" (0x1234));
> >  		return e;
> >  	    }
> >  	} "-mfuture"]
> 
> You are still passing -mfuture.

All of the tests use the -m<flag> test and not -mcpu=<cpu> test
(i.e. -mpower9-vector instead of -mcpu=power9 or -mdejagnu-cpu=power9), so I
was being consistant with those tests.

> > +# Return 1 if this is a PowerPC target supporting -mcpu=future.  The compiler
> > +# must support large numeric prefixed addresses by default when -mfuture is
> > +# used.  We test loading up a large constant to verify that the full 34-bit
> > +# offset for prefixed instructions is supported and we check for a prefixed
> > +# load as well.
> 
> The comment says one thing.
> 
> -mfuture isn't a compiler option...  Well it still is, but that should
> be removed.

Again, I was just being consistant with the other tests.

> The actual test uses only 30 bits (and a positive number).  Which is fine,
> but the comment is misleading then: the code doesn't test "if the full
> 34-bit offset is supported".

I can fix the comment.

> I don't understand why we test both pli and pld.

Just being cautious.

> > +proc check_effective_target_powerpc_prefixed_addr_ok { } {
> 
> The name says another.
> 
> > +    if { ([istarget powerpc*-*-*]) } {
> 
> This part has no function?  Are there any testcases that test for our
> prefixed insns that are compiler for non-powerpc?

Probably not, but all of the other tests have the same prefix.

> If we want this at all, this test shouldn't be nested, just should be
> an early-out.
> 
> > +	return [check_no_compiler_messages powerpc_prefixed_addr_ok object {
> > +	    int main (void) {
> > +		extern long l[];
> > +		long e, e2;
> > +		asm ("pli %0,%1" : "=r" (e) : "n" (0x12345678));
> > +		asm ("pld %0,0x12345678(%1)" : "=r" (e2) : "r" (& l[0]));
> 
> (should be "b" for the last constraint; and "&l[0]" is usually written
> just "l").
> 
> > +		return e - e2;
> > +	    }
> > +	} "-mfuture"]
> 
> And the code tests two things.
> 
> -mcpu=future, instead?
>
> Does this need to be separate from check_effective_target_powerpc_future_ok
> at all?

This gets back to whether some port will eventually want to use FUTURE
instructions in a 32-bit context.  Linux will always be 64-bit little endian,
but there are other platforms out there that may want to generate FUTURE code.

> > +# Return 1 if this is a PowerPC target supporting -mfuture.  The compiler must
> 
> That is the third selector claiming to test the same thing ("target supports
> -mfuture").
> 
> > +# support PC-relative addressing when -mcpu=future is used to pass this test.
> > +
> > +proc check_effective_target_powerpc_pcrel_ok { } {
> > +    if { ([istarget powerpc*-*-*]) } {
> > +	return [check_no_compiler_messages powerpc_pcrel_ok object {
> > +	      int main (void) {
> > +		  static int s __attribute__((__used__));
> > +		  int e;
> > +		  asm ("plwa %0,s@pcrel(0),1" : "=r" (e));
> > +		  return e;
> > +	      }
> > +	  } "-mfuture"]
> > +      } else {
> > +	  return 0
> > +      }
> > +}
> 
> So every assembler will support either all three of these, or none.
> Can you simplify this please?

I can imagine for example AIX might support 64-bit 'future' but not support
PC-relative.  I believe you (or David) asked to split up the prefixed
addressing with numeric offets and PC-relative addressing, because ports might
not be able to support PC-relative addressing.
Segher Boessenkool Dec. 3, 2019, 7:52 p.m. UTC | #3
On Tue, Dec 03, 2019 at 01:11:55PM -0500, Michael Meissner wrote:
> On Fri, Nov 22, 2019 at 08:11:16PM -0600, Segher Boessenkool wrote:
> > On Thu, Nov 14, 2019 at 05:56:50PM -0500, Michael Meissner wrote:
> > > 	* lib/target-supports.exp
> > > 	(check_effective_target_powerpc_future_ok): Do not require 64-bit
> > > 	or Linux support before doing the test.  Use a 32-bit constant in
> > > 	PLI.
> > 
> > You changed from 0x12345 to 0x1234, instead -- why?
> 
> The last time I did the patch there was discussion about 32-bit support.  I
> just rewrote the patch to accomidate possibly using -mcpu=future on a 32-bit
> platform.  In particular, AIX.  So I rewrote the basic test so that in theory
> it would run on a 32-bit platform.

I don't see how this has anything to with it?  The insns was just fine
for 32-bit already?

> > > +# Return 1 if this is a PowerPC target supporting -mcpu=future.

> > >  proc check_effective_target_powerpc_future_ok { } {
> > > -    if { ([istarget powerpc64*-*-linux*]) } {
> > > +    if { ([istarget powerpc*-*-*]) } {
> > >  	return [check_no_compiler_messages powerpc_future_ok object {
> > >  	    int main (void) {
> > >  		long e;
> > > -		asm ("pli %0,%1" : "=r" (e) : "n" (0x12345));
> > > +		asm ("pli %0,%1" : "=r" (e) : "n" (0x1234));
> > >  		return e;
> > >  	    }
> > >  	} "-mfuture"]
> > 
> > You are still passing -mfuture.
> 
> All of the tests use the -m<flag> test and not -mcpu=<cpu> test
> (i.e. -mpower9-vector instead of -mcpu=power9 or -mdejagnu-cpu=power9), so I
> was being consistant with those tests.

Yes, that is a nasty problem, so let's not promulgate that.  Anyway, it
also contradicts the comment on this function.

> > I don't understand why we test both pli and pld.
> 
> Just being cautious.

Hrm, the check_effective_target_powerpc_future_ok test should probably
use a non-prefixed instruction?  If we cannot do that right now, add a
comment?

> > > +proc check_effective_target_powerpc_prefixed_addr_ok { } {
> > 
> > The name says another.
> > 
> > > +    if { ([istarget powerpc*-*-*]) } {
> > 
> > This part has no function?  Are there any testcases that test for our
> > prefixed insns that are compiler for non-powerpc?
> 
> Probably not, but all of the other tests have the same prefix.

Feel free to send patches fixing old mistakes, too.

> > > +	return [check_no_compiler_messages powerpc_prefixed_addr_ok object {

> > Does this need to be separate from check_effective_target_powerpc_future_ok
> > at all?
> 
> This gets back to whether some port will eventually want to use FUTURE
> instructions in a 32-bit context.  Linux will always be 64-bit little endian,

No, Linux also supports 32-bit BE and 64-bit BE.  Even if we do not
support Future on those currently, this may change.  There is absolutely
no reason to make it harder to do later.  If we think some combinations
do not exist we can *ignore* them.  That is much less work, too.

> but there are other platforms out there that may want to generate FUTURE code.

But we should not anticipate those having broken assemblers that do not
support half of the insns, etc.  You *already* required prefixed insns in
the base Future test, to prove how futile this is.

> > > +# support PC-relative addressing when -mcpu=future is used to pass this test.
> > > +
> > > +proc check_effective_target_powerpc_pcrel_ok { } {
> > > +    if { ([istarget powerpc*-*-*]) } {
> > > +	return [check_no_compiler_messages powerpc_pcrel_ok object {
> > > +	      int main (void) {
> > > +		  static int s __attribute__((__used__));
> > > +		  int e;
> > > +		  asm ("plwa %0,s@pcrel(0),1" : "=r" (e));
> > > +		  return e;
> > > +	      }
> > > +	  } "-mfuture"]
> > > +      } else {
> > > +	  return 0
> > > +      }
> > > +}
> > 
> > So every assembler will support either all three of these, or none.
> > Can you simplify this please?
> 
> I can imagine for example AIX might support 64-bit 'future' but not support
> PC-relative.  I believe you (or David) asked to split up the prefixed
> addressing with numeric offets and PC-relative addressing, because ports might
> not be able to support PC-relative addressing.

That is fine, but these tests do not do that.  Some clearer comments will
help, too.


Segher

Patch
diff mbox series

Index: gcc/testsuite/lib/target-supports.exp
===================================================================
--- gcc/testsuite/lib/target-supports.exp	(revision 278173)
+++ gcc/testsuite/lib/target-supports.exp	(working copy)
@@ -5345,16 +5345,14 @@  proc check_effective_target_powerpc_p9mo
     }
 }
 
-# Return 1 if this is a PowerPC target supporting -mfuture.
-# Limit this to 64-bit linux systems for now until other
-# targets support FUTURE.
+# Return 1 if this is a PowerPC target supporting -mcpu=future.
 
 proc check_effective_target_powerpc_future_ok { } {
-    if { ([istarget powerpc64*-*-linux*]) } {
+    if { ([istarget powerpc*-*-*]) } {
 	return [check_no_compiler_messages powerpc_future_ok object {
 	    int main (void) {
 		long e;
-		asm ("pli %0,%1" : "=r" (e) : "n" (0x12345));
+		asm ("pli %0,%1" : "=r" (e) : "n" (0x1234));
 		return e;
 	    }
 	} "-mfuture"]
@@ -5363,6 +5361,46 @@  proc check_effective_target_powerpc_futu
     }
 }
 
+# Return 1 if this is a PowerPC target supporting -mcpu=future.  The compiler
+# must support large numeric prefixed addresses by default when -mfuture is
+# used.  We test loading up a large constant to verify that the full 34-bit
+# offset for prefixed instructions is supported and we check for a prefixed
+# load as well.
+
+proc check_effective_target_powerpc_prefixed_addr_ok { } {
+    if { ([istarget powerpc*-*-*]) } {
+	return [check_no_compiler_messages powerpc_prefixed_addr_ok object {
+	    int main (void) {
+		extern long l[];
+		long e, e2;
+		asm ("pli %0,%1" : "=r" (e) : "n" (0x12345678));
+		asm ("pld %0,0x12345678(%1)" : "=r" (e2) : "r" (& l[0]));
+		return e - e2;
+	    }
+	} "-mfuture"]
+    } else {
+	return 0
+    }
+}
+
+# Return 1 if this is a PowerPC target supporting -mfuture.  The compiler must
+# support PC-relative addressing when -mcpu=future is used to pass this test.
+
+proc check_effective_target_powerpc_pcrel_ok { } {
+    if { ([istarget powerpc*-*-*]) } {
+	return [check_no_compiler_messages powerpc_pcrel_ok object {
+	      int main (void) {
+		  static int s __attribute__((__used__));
+		  int e;
+		  asm ("plwa %0,s@pcrel(0),1" : "=r" (e));
+		  return e;
+	      }
+	  } "-mfuture"]
+      } else {
+	  return 0
+      }
+}
+
 # Return 1 if this is a PowerPC target supporting -mfloat128 via either
 # software emulation on power7/power8 systems or hardware support on power9.
 
@@ -7261,6 +7299,7 @@  proc is-effective-target { arg } {
 	  "named_sections" { set selected [check_named_sections_available] }
 	  "gc_sections"    { set selected [check_gc_sections_available] }
 	  "cxa_atexit"     { set selected [check_cxa_atexit_available] }
+	  "powerpc_future_hw" { set selected [check_powerpc_future_hw_available] }
 	  default          { error "unknown effective target keyword `$arg'" }
 	}
     }