On Wed, Sep 01, 2021 at 04:22:13PM -0400, Michael Meissner wrote:
> On Tue, Aug 31, 2021 at 06:41:30PM -0500, Segher Boessenkool wrote:
> > Hi!
> >
> > Please do two separate patches. The first that adds the instruction
> > (with a bit pattern, i.e. integer, input), and perhaps a second pattern
> > that has an fp as input and uses it if the constant is valid for the
> > insn (survives being converted to SP and back to DP (or the other way
> > around), and is not denormal). That can be two patches if you want,
> > but :-)
>
> Well we already have the two stages for the built-in. But the point of the
> work is to make it work without the explicit builtin.
There should be a define_insn that works with just as just a bit
pattern, without floating point in sight.
> > Having the integer intermediate step not only makes the code hugely less
> > complicated, but is also allows e.g.
>
>
> > ===
> > typedef unsigned long long v2u64 __attribute__ ((vector_size (16)));
> > v2u64 f(void)
> > {
> > v2u64 x = { 0x8000000000000000, 0x8000000000000000 };
> > return x;
> > }
> > ===
> >
> > to be optimised properly.
> >
> > The second part is letting the existing code use such FP (and integer!)
> > contants.
>
> It is somewhat harder to do this as integer values, because you have to check
> whether all of the bits are correct, including skipping the SF denormal bits.
You have to *anyway*. And it is easy enough.
> I tend to think in real life, the only win will be be -0.0
> (i.e. 0x8000000000000000 that you mentioned). I doubt many V2DI constants
> will
> be appropriate canidates for this.
There are a few more. But the point is that the code is *simpler* like
this. It usually is -- shortcuts often take more time in the end.
> > (Btw, initialising the value (although the function always writes it) is
> > not defensive programming, it is hiding bugs. IMNSHO :-) )
>
> And avoiding warnings.
Shutting up warnings that your control flow is not good is not a good
idea usually. Don't fight the compiler: it does not matter if you have
better taste than it, it will win. Instead, write better control flow.
There might even be an actual bug hiding in there!
> > > +/* Whether a permute type instruction is a prefixed instruction. This is
> > > + called from the prefixed attribute processing. */
> > > +
> > > +bool
> > > +prefixed_permute_p (rtx_insn *insn)
> >
> > What does this have to do with this patch?
>
> This is used by the prefixed type attribute to know that the
> xxsplti{w,dp,32dx}
> instructions are prefixed by default, but unlike paddi, they don't want a
> leading 'p' in the instruction.
They are not permute insns, that's where your code turns into an enigma.
You should not have a function for prefixed insns that are not a variant
of non-prefixed insns: that is *most* prefixed insns (eventually it will
be, right now you see memory insns mostly of course). Pick out the
special case, instead (hint: if it has a "not" in the description, you
probably picked the wrong side).
So you have an "is_prefixed" thing, all prefixed insns, and an
"is_prefixed_variant" (or some better name :-) ) function, that returns
true for those prefixed insns written with a leading "p" on an existing
base insn (and actually have the original opcode as suffix).
> > > @@ -7755,15 +7760,16 @@ (define_insn "movsf_hardfloat"
> > > @@ -8051,20 +8057,21 @@ (define_insn "*mov<mode>_hardfloat32"
> > > @@ -8091,19 +8098,19 @@ (define_insn "*mov<mode>_softfloat32"
> > > @@ -8125,18 +8132,19 @@ (define_insn "*mov<mode>_hardfloat64"
> > > @@ -8170,6 +8178,7 @@ (define_insn "*mov<mode>_softfloat64"
> >
> > It would be a good idea to merge many of these patterns again. We can
> > do this now that we have the "isa" and "enabled" attributes.
>
> I don't plan to do this right now.
The longer you wait the more work it becomes, and the more work other
things (like this patch) become as well.
Segher
