Add VIS intrinsics header for sparc.

Hans, here is what I'm playing with right now against current
trunk.

I looked at the use cases for making use of the scale factor in the
VIS %gsr register and it's used similar to how rounding modes are
modified in the FPU control register.

You have a function, or family of functions, that want to operate with
a certain scale factor.  And at the top level the first thing you do
is set the %gsr as you want it to be set.

So I've added a GSR register to the sparc backend and then added
__vis_write_gsr() and __vis_read_gsr() functions to facilitate the
use cases I've seen.

This allowed me to describe to the compiler exactly what the alignaddr
instructions do, and thus the unspecs for them are now gone.

The pack and faligndata intrinsics still need to be unspec, and thus I
merely added GSR uses to those patterns which is enough to let the
compiler get the dataflow right.

This all seems sufficient for what things like Sun's medialib and your
RAPP project want to do.

I'll look into your other suggestion in PR48974, namely making use of
fone VIS instructions.

Thanks.

On Sat, 24 Sep 2011, David Miller wrote:
> Hans, here is what I'm playing with right now against current
> trunk.

A spot-check review:

> I looked at the use cases for making use of the scale factor in the
> VIS %gsr register and it's used similar to how rounding modes are
> modified in the FPU control register.

It's more of a parameter actually, GSR.scale_factor is the
bit-shift count for the pack insns and GSR.alignaddr_offset the
byte-shift in the aligndata insns.

> You have a function, or family of functions, that want to operate with
> a certain scale factor.  And at the top level the first thing you do
> is set the %gsr as you want it to be set.

Certainly an improvement, but...

> So I've added a GSR register to the sparc backend and then added
> __vis_write_gsr() and __vis_read_gsr() functions to facilitate the
> use cases I've seen.

I'd prefer it as a parameter to the builtins (expanding to two
insns, letting gcc get rid of the redundant ones; let the
initialization value be 0).  I understand you're trying to keep
some kind of compatibility there, but additional builtins would
do the trick and fit nicely: the new builtins expanding to a set
of GSR (GSR field) followed by the "old" insn but fixed as in
this patch.  Besides, the functions that use GSR still can't be
const in this patch.  I guess they never can, when you think of
it, setting and/or using a register that can affect/be affected
something elsewhere, when that something is known to gcc.  Oh well.

Another aspect would be to model the different GSR fields as
different registers; they're used completely differently and
just happen to be set with the same insn.  That might help gcc
getting rid of redundant settings.

> This allowed me to describe to the compiler exactly what the alignaddr
> instructions do, and thus the unspecs for them are now gone.
>
> The pack and faligndata intrinsics still need to be unspec,

FWIW not "need"; IIUC at least faligndata *can* be a vec_select
of a vec_concat of the two vectors, but in practice I don't
think gcc can make use of it yet and all ports use unspec...

While on faligndata, see vec_realign_load_<mode> (sadly
undocumented at present); it'll enable the autovectorizer to...
autovectorize some more.  (Right, I'm working on [yet] another
SIMD back-end, implemented as MIPS COP2 insns.)

> and thus I
> merely added GSR uses to those patterns which is enough to let the
> compiler get the dataflow right.

How about putting it inside the unspec vector?  Those "use"
thingies always gives me the creeps; outside of an insn (no, not
here) they're sometimes lost and at least disconnected to the
insn.  I think practically there's no difference here.

> This all seems sufficient for what things like Sun's medialib and your
> RAPP project want to do.
>
> I'll look into your other suggestion in PR48974, namely making use of
> fone VIS instructions.

One more: please consider adding a
 if (TARGET_VIS) builtin_define ("__VIS__=something") so I as a
user theoretically wouldn't *have* to autoconfiscate for the
changes. ;)

> +  def_builtin_const ("__builtin_vis_fpack16", CODE_FOR_fpack16_vis,
> +		     v4qi_ftype_v4hi);
> +  def_builtin_const ("__builtin_vis_fpack32", CODE_FOR_fpack32_vis,
> +		     v8qi_ftype_v2si_v8qi);
> +  def_builtin_const ("__builtin_vis_fpackfix", CODE_FOR_fpackfix_vis,
> +		     v2hi_ftype_v2si);
>    def_builtin_const ("__builtin_vis_fexpand", CODE_FOR_fexpand_vis,
>  		     v4hi_ftype_v4qi);

No, they (and aligndata) can't be const as long as they're
affected by something other than their parameters (GSR); pure
yes but not const.  See extend.texi.


> +      def_builtin_const ("__builtin_vis_alignaddr", CODE_FOR_alignaddrdi_vis,
> +			 ptr_ftype_ptr_di);
> +      def_builtin_const ("__builtin_vis_alignaddrl", CODE_FOR_alignaddrldi_vis,
> +			 ptr_ftype_ptr_di);

Can't be neither pure nor const; affects something global (GSR).

BTW, vector header files are overrated, at least when there's no
compiler compatibility expected.  They can even be in the way:
there's an ARM NEON PR being stalled because of concern that the
header could be used with another gcc version.  Bah. ...ok I see
visintrin.h is already in.  Never mind then. :)

brgds, H-P

From: Hans-Peter Nilsson <hp@bitrange.com>
Date: Sat, 24 Sep 2011 17:15:06 -0400 (EDT)

> It's more of a parameter actually, GSR.scale_factor is the
> bit-shift count for the pack insns and GSR.alignaddr_offset the
> byte-shift in the aligndata insns.

I realize this.

> I'd prefer it as a parameter to the builtins (expanding to two
> insns, letting gcc get rid of the redundant ones; let the
> initialization value be 0).  I understand you're trying to keep
> some kind of compatibility there, but additional builtins would
> do the trick and fit nicely: the new builtins expanding to a set
> of GSR (GSR field) followed by the "old" insn but fixed as in
> this patch.  Besides, the functions that use GSR still can't be
> const in this patch.  I guess they never can, when you think of
> it, setting and/or using a register that can affect/be affected
> something elsewhere, when that something is known to gcc.  Oh well.

I read this idea in your PR before I did this work and I disagree that
this is a better approach, because then I have to assume that you care
about all the other bits in the %gsr register.

So on the first set I'd have to read it, mask it out, then set the
scale bits.  A needless waste of 20 to 30 cycles on UltraSPARC-III.

If you just call "__vis_write_gsr()" at the beginning of your kernel,
you can tell the compiler that you just want to set the scaling bits
and you don't care about the others at all.

> Another aspect would be to model the different GSR fields as
> different registers; they're used completely differently and
> just happen to be set with the same insn.  That might help gcc
> getting rid of redundant settings.

Again, this doesn't allow the user to say "don't care" about the other
fields like a plain "__vis_write_gsr(2<<3)" call does.

You know what fields actually matter for your code.

> FWIW not "need"; IIUC at least faligndata *can* be a vec_select
> of a vec_concat of the two vectors, but in practice I don't
> think gcc can make use of it yet and all ports use unspec...
> 
> While on faligndata, see vec_realign_load_<mode> (sadly
> undocumented at present); it'll enable the autovectorizer to...
> autovectorize some more.  (Right, I'm working on [yet] another
> SIMD back-end, implemented as MIPS COP2 insns.)

Thanks for these suggestions.

> How about putting it inside the unspec vector?  Those "use"
> thingies always gives me the creeps; outside of an insn (no, not
> here) they're sometimes lost and at least disconnected to the
> insn.  I think practically there's no difference here.

The canonical thing to do is to put them outside of the unspec
so that is what I have done.

> One more: please consider adding a
>  if (TARGET_VIS) builtin_define ("__VIS__=something") so I as a
> user theoretically wouldn't *have* to autoconfiscate for the
> changes. ;)

This is on my todo list as well, I'll try to emit some CPP define
compatible with what Sun uses.  But, thanks for reminding me.

>> +  def_builtin_const ("__builtin_vis_fpack16", CODE_FOR_fpack16_vis,
>> +		     v4qi_ftype_v4hi);
>> +  def_builtin_const ("__builtin_vis_fpack32", CODE_FOR_fpack32_vis,
>> +		     v8qi_ftype_v2si_v8qi);
>> +  def_builtin_const ("__builtin_vis_fpackfix", CODE_FOR_fpackfix_vis,
>> +		     v2hi_ftype_v2si);
>>    def_builtin_const ("__builtin_vis_fexpand", CODE_FOR_fexpand_vis,
>>  		     v4hi_ftype_v4qi);
> 
> No, they (and aligndata) can't be const as long as they're
> affected by something other than their parameters (GSR); pure
> yes but not const.  See extend.texi.

Good catch, I was thinking purely on the RTL level where we do show
the compiler all of the "inputs" but at the tree level this is not
visible.

I'll fix that up for the next revision.

>> +      def_builtin_const ("__builtin_vis_alignaddr", CODE_FOR_alignaddrdi_vis,
>> +			 ptr_ftype_ptr_di);
>> +      def_builtin_const ("__builtin_vis_alignaddrl", CODE_FOR_alignaddrldi_vis,
>> +			 ptr_ftype_ptr_di);
> 
> Can't be neither pure nor const; affects something global (GSR).

Gotcha.

I'd like to revisit this at some point in the future though, maybe we
can legitimately at least mark these things pure.

On Sat, 24 Sep 2011, David Miller wrote:
> From: Hans-Peter Nilsson <hp@bitrange.com>
> Date: Sat, 24 Sep 2011 17:15:06 -0400 (EDT)
> > I'd prefer it as a parameter to the builtins (expanding to two
> > insns, letting gcc get rid of the redundant ones; let the
> > initialization value be 0).  I understand you're trying to keep
> > some kind of compatibility there, but additional builtins would
> > do the trick and fit nicely: the new builtins expanding to a set
> > of GSR (GSR field) followed by the "old" insn but fixed as in
> > this patch.  Besides, the functions that use GSR still can't be
> > const in this patch.  I guess they never can, when you think of
> > it, setting and/or using a register that can affect/be affected
> > something elsewhere, when that something is known to gcc.  Oh well.
>
> I read this idea in your PR before I did this work and I disagree that
> this is a better approach, because then I have to assume that you care
> about all the other bits in the %gsr register.

I don't understand what you mean here.  Maybe it doesn't
matter...  My suggestions come from observing what gcc did to
the "faked gsr modelling" I had to use with the current releases
(what moving and eliminating redundant variable settings used in
asms that it did; turned out acceptable FWIW, no redundant
reads), which would map directly to my suggestion.  But I guess
you have a point in that your setting-gsr-then-using-builtins
maps better to the machine insns.

BTW, don't forget to clobber GSR at call insns!

> So on the first set I'd have to read it, mask it out, then set the
> scale bits.  A needless waste of 20 to 30 cycles on UltraSPARC-III.

No, it doesn't have to be read.  If the fields have (useful)
implicit initial values (like scale=7 and align=4) at the
beginning of any function, you wouldn't have to read and mask,
just set.  (Caveat: the port has to have a way to emit a
gsr-setting even if the supposed-initial-values are specified -
like another register or variable, or the initial-value
machinery as I suggested.)

> If you just call "__vis_write_gsr()" at the beginning of your kernel,
> you can tell the compiler that you just want to set the scaling bits
> and you don't care about the others at all.

Don't care how?  They're certainly set by both __vis_write_gsr()
and alignaddr and used by faligndata.  I guess my confusion is
that I don't see what aspect is "don't care" here that'd be
"care" with my suggestion.

> > Another aspect would be to model the different GSR fields as
> > different registers; they're used completely differently and
> > just happen to be set with the same insn.  That might help gcc
> > getting rid of redundant settings.
>
> Again, this doesn't allow the user to say "don't care" about the other
> fields like a plain "__vis_write_gsr(2<<3)" call does.

But that'd set GSR.alignaddr_offset to 0 rather than "don't
care".

> You know what fields actually matter for your code.

A good reason to model them as different registers!

Still, this is a good start and much more workable (and
schedulable) than what's already there, thank you for that.
It doesn't add hurdles for a revisit, if the mechanism is found
unusable or the generated code pessimal!

brgds, H-P

From: Hans-Peter Nilsson <hp@bitrange.com>
Date: Sat, 24 Sep 2011 18:37:33 -0400 (EDT)

> BTW, don't forget to clobber GSR at call insns!

This I explicitly want to avoid and is an explicit design decision.

Like I said the model is like setting the floating point rounding mode
for a family of functions.

You set the floating point rounding mode at the top level, run your
kernel and all the helper functions in that mode.

The %gsr scaling factor is to be used similarly.

You have to control all the functions that get called once you set the
%gsr before a calculation, and they either have to explicitly save and
restore the %gsr around changes to %gsr, or have been designed to use
the %gsr setting made by the callee.

The last thing I want to do is have to teach reload how to handle this
thing, it simply makes no sense to put that much engineering into it
if it is for zero or very little gain.

And it would explicitly prevent the kind of model I see as the most
reasonable for using this register, in that if we clobber it during
a call there is no way for the user to say not to save and restore
%gsr over a call.

>> So on the first set I'd have to read it, mask it out, then set the
>> scale bits.  A needless waste of 20 to 30 cycles on UltraSPARC-III.
> 
> No, it doesn't have to be read.  If the fields have (useful)
> implicit initial values (like scale=7 and align=4) at the
> beginning of any function, you wouldn't have to read and mask,
> just set.

You can't just set.  What about the VIS-2.0 byte-mask at the top
32-bits of the register, are you just going to clobber that when you
change the scale factor?

If we support treating the different fields as different registers we
have to preserve the setting of the other fields of %gsr when we
change one of them.  There are 5 fields currently defined:

1) align address <2:0>
2) scale factor <7:3>
3) interval rounding mode (VIS 2.0) <26:25>
4) interval mode enable <27>
5) Byte mask (VIS 2.0) <63:32>

And also this idea of using get_hard_reg_initial_val() to "optimize"
this kind of usage especially forces us to clobber the %gsr over
function calls which, as stated, I want to avoid if at all possible.

>> Again, this doesn't allow the user to say "don't care" about the other
>> fields like a plain "__vis_write_gsr(2<<3)" call does.
> 
> But that'd set GSR.alignaddr_offset to 0 rather than "don't
> care".

Zero is equivalent to "don't care" in this situation if either
1) you aren't doing any falignaddr operations or 2) you are
then going to subsequently do an "alignaddr" to set that field
up.

Look at the medialib code, that's basically the usage model there
and I think it's quite reasonable.

> It doesn't add hurdles for a revisit, if the mechanism is found
> unusable or the generated code pessimal!

Absolutely, thanks for your review.

On Sat, 24 Sep 2011, David Miller wrote:

> From: Hans-Peter Nilsson <hp@bitrange.com>
> Date: Sat, 24 Sep 2011 18:37:33 -0400 (EDT)
>
> > BTW, don't forget to clobber GSR at call insns!
>
> This I explicitly want to avoid and is an explicit design decision.

Aha, now I get it; that's certainly key.  Thanks for taking the time.

Yes, it's certainly more flexible to have the user set GSR than
allowing gcc to clobber it when seeing VIS intrinsics, at the
minor usability cost of the user having to keep track of GSR
separately to when used in the individual intrinsics.

> Like I said the model is like setting the floating point rounding mode
> for a family of functions.

Aha 2: I didn't interpret what you wrote as referring to the
model; I thought you meant the actual function (one of the
usages of the fpack insns being "fixed math").  Sure.

> Zero is equivalent to "don't care" in this situation if either
> 1) you aren't doing any falignaddr operations or 2) you are

(JFTR, "faligndata")

> then going to subsequently do an "alignaddr" to set that field
> up.

brgds, H-P
PS. gcc-4.7/changes.html?

From: Hans-Peter Nilsson <hp@bitrange.com>
Date: Sat, 24 Sep 2011 19:32:55 -0400 (EDT)

> PS. gcc-4.7/changes.html?

Also on my TODO list, and Eric made some noise about documenting these
improvements as well, thanks for the reminder.

I'll post and commit the current version of my %gsr changes after my
bootstrap/testsuite run finishes.

From: David Miller <davem@davemloft.net>
Date: Sat, 24 Sep 2011 20:05:19 -0400 (EDT)

> From: Hans-Peter Nilsson <hp@bitrange.com>
> Date: Sat, 24 Sep 2011 19:32:55 -0400 (EDT)
> 
>> PS. gcc-4.7/changes.html?
> 
> Also on my TODO list, and Eric made some noise about documenting these
> improvements as well, thanks for the reminder.

I just commited an update to the wwwdocs.