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
