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.
--
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meiss...@linux.ibm.com, phone: +1 (978) 899-4797
