| 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. | expand |
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
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.
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
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'" } } }