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