Hi!
On Thu, Feb 10, 2022 at 04:28:02PM -0600, Bill Schmidt wrote:
> On 2/10/22 4:11 PM, Segher Boessenkool wrote:
> >> 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.
Yes, I missed it. That is not an argument that it would be good or
should not be change.
> I don't really understand why you want
> it changed now.
Because it is wrong.
> You must not be looking at the most recent trunk revision.
Indeed I haven't been able to update master for a week or so, it does
not bootstrap, as we have talked about.
> >> 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.
Things that were added in stage 4, a few days ago even. Things that are
broken and wrong. Things I do not want to have to release with and deal
with all the pain of having broken released versions.
> This is a purely arbitrary choice.
No, it is not. It flies in the face of consistency.
> 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.
The one that corresponds to the name should be the default. I don't see
how you can argue otherwise.
> 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.
Everything in our backend is BE by default, just like everything in the
architecture is. Yes, LE works almost as well (or just as well) in most
places, but everything is named assuming BE. This consistency is hugely
important, without it the reader will not understand things as well and
as easily.
> >> 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.
Thanks!
> > The other things need fixing before the patch is okay.
>
> I'd ask you to reconsider, as explained above.
It is purely an implementation thing, and it is completely trivial to
do. If you truly are afraid of breaking things (you should not be), it
is marginally acceptable to do this as the very first thing in stage 1.
Consistency matters. Naming matters. These shape how we think about
things.
Segher
