Hi!

As Will says, the subject is much too long.  50 chars max is the ideal.
"target supports" isn't clear; maybe

        rs6000: Add effective targets powerpc_{pcrel,prefixed_addr}

or something like it (prefix, colon, capital, no dot).


On Mon, Apr 27, 2020 at 03:46:06PM -0400, Michael Meissner wrote:
> 2020-04-27  Michael Meissner  <meiss...@linux.ibm.com>
> 
>       * lib/target-supports.exp (check_effective_target_powerpc_pcrel):
>       New target for PowerPC -mcpu=future support.

This isn't a "new target", it's not a "target" after all?  Just say
"New.", you can't go wrong with that (if it is new, heh).

>       (check_effective_target_powerpc_prefixed_addr): New target for
>       PowerPC -mcpu=future support.

You also say the same thing for both new functions, which cannot be
right.

> +# Return 1 if the target generates PC-relative instructions automatically
> +proc check_effective_target_powerpc_pcrel { } {
> +    return [check_no_messages_and_pattern powerpc_pcrel \
> +     {\mpld\M.*[@]pcrel} assembly {
> +         static long s;
> +         long *p = &s;
> +         long foo (void) { return s; }
> +     } {-O2 -mcpu=future}]
> +}

A comment on the "long *p" line wouldn't hurt ;-)

Does the "@" need quoting like that?

Do you want to test for the exact word "pcrel", or any word starting
with it?

"If the target generates pcrel automatically"...  well, your test passes
an -mcpu= that makes pcrel possible.  This needs a different description,
maybe a different name, or maybe the test should change?  How is this
effective target used?

> +# Return 1 if the target generates prefixed instructions automatically
> +proc check_effective_target_powerpc_prefixed_addr { } {
> +    return [check_no_messages_and_pattern powerpc_prefixed_addr \
> +     {\mpld\M} assembly {
> +         long foo (long *p) { return p[0x12345]; }
> +     } {-O2 -mcpu=future}]
> +}

Similar here.


Segher

Reply via email to