Hi!
On 2/10/22 4:11 PM, Segher Boessenkool wrote:
> On Thu, Feb 10, 2022 at 03:17:05PM -0600, Bill Schmidt wrote:
>>>> /* 1 argument vector functions added in ISA 3.0 (power9). */
>>>> -BU_P9V_AV_1 (VCLZLSBB_V16QI, "vclzlsbb_v16qi", CONST, vclzlsbb_v16qi)
>>>> -BU_P9V_AV_1 (VCLZLSBB_V8HI, "vclzlsbb_v8hi", CONST, vclzlsbb_v8hi)
>>>> -BU_P9V_AV_1 (VCLZLSBB_V4SI, "vclzlsbb_v4si", CONST, vclzlsbb_v4si)
>>>> -BU_P9V_AV_1 (VCTZLSBB_V16QI, "vctzlsbb_v16qi", CONST, vctzlsbb_v16qi)
>>>> -BU_P9V_AV_1 (VCTZLSBB_V8HI, "vctzlsbb_v8hi", CONST, vctzlsbb_v8hi)
>>>> -BU_P9V_AV_1 (VCTZLSBB_V4SI, "vctzlsbb_v4si", CONST, vctzlsbb_v4si)
>>>> +BU_P9V_AV_1 (VCLZLSBB_V16QI, "vclzlsbb_v16qi", CONST, vctzlsbb_v16qi)
>>>> +BU_P9V_AV_1 (VCLZLSBB_V8HI, "vclzlsbb_v8hi", CONST, vctzlsbb_v8hi)
>>>> +BU_P9V_AV_1 (VCLZLSBB_V4SI, "vclzlsbb_v4si", CONST, vctzlsbb_v4si)
>>>> +BU_P9V_AV_1 (VCTZLSBB_V16QI, "vctzlsbb_v16qi", CONST, vclzlsbb_v16qi)
>>>> +BU_P9V_AV_1 (VCTZLSBB_V8HI, "vctzlsbb_v8hi", CONST, vclzlsbb_v8hi)
>>>> +BU_P9V_AV_1 (VCTZLSBB_V4SI, "vctzlsbb_v4si", CONST, vclzlsbb_v4si)
>>> Please change the default to be equal to the builtin name, so, the BE
>>> version. We do that everywhere else as well, and it makes a lot more
>>> sense (since everything in Power has BE numbering).
>>>
>>> The trunk version has this correct afaics?
>> No, trunk has this, for example:
>>
>> const signed int __builtin_altivec_vclzlsbb_v16qi (vsc);
>> VCLZLSBB_V16QI vctzlsbb_v16qi {endian}
> I see this on trunk:
>
> const signed int __builtin_altivec_vclzlsbb_v16qi (vsc);
> VCLZLSBB_V16QI vclzlsbb_v16qi {}
>
> Oh, you changed it? Please fix it, then.
In a patch you approved, yes. I don't really understand why you want
it changed now. You must not be looking at the most recent trunk
revision.
>
>> Throughout the new builtin infrastructure, the defaults are set for
>> little-endian, and the "endian" flag changes behavior for big-endian.
> That is a big mistake. There are many machine instructions that are
> *always* big-endian (most even!), and none that are always
> little-endian. So this should be fixed, sooner rather than later :-(
That does not seem like a good idea in stage 4 to me. That requires
yet another patch to reverse a bunch of other things unnecessarily.
This is a purely arbitrary choice. The endian flag is only used when
a built-in function must have one behavior for big-endian, and another
behavior for little-endian. Which one is chosen as the default is
absolutely arbitrary. When we expand the built-in we will either
accept the default or change to the other. The existence of machine
instructions that are only big-endian has nothing to do with the case;
what matters is the existence of built-in functions that have two
behaviors.
>>>> /* { dg-require-effective-target powerpc_p9vector_ok } */
>>>> /* { dg-options "-mdejagnu-cpu=power9" } */
>>>> +/* { dg-additional-options "-mbig" { target powerpc64le-*-* } } */
>>> You don't need the target clause, if it already is BE by default it does
>>> not do anything to add it redundantly.
>>>
>>> But this is wrong anyway: the name of the target triple does not say
>>> whether we are BE or LE. Instead you should use the be or le selectors.
>>> But again, just add -mbig always.
>> This was added by David Edelsohn to the trunk version of the patch, because
>> -mbig actually is not supported on all subtargets. (I found that quite
>> surprising also.)
> Huh. Yeah I think I encountered that before.
>
> So this is because these options are in sysv4.opt .
>
>> Apparently this doesn't work on AIX, for example. But
>> -mlittle works everywhere. Go figure.
> ... and -mlittle is exactly the same? Wtw.
>
> I only looked at the .opt files, maybe one of them is handled directly,
> or more likely in specs? And not symmetrically?
>
>> That's something that should be fixed, I guess, but it's orthogonal
>> to this patch.
> Fixing it later is more work :-(
>
> Please at least open a bug report for it.
I can do that.
>
>
> The other things need fixing before the patch is okay.
I'd ask you to reconsider, as explained above.
Thanks,
Bill
>
>
> Segher
