From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8981 invoked by alias); 24 Sep 2011 21:31:34 -0000 Received: (qmail 8967 invoked by uid 22791); 24 Sep 2011 21:31:33 -0000 X-SWARE-Spam-Status: No, hits=-1.7 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from shards.monkeyblade.net (HELO shards.monkeyblade.net) (198.137.202.13) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 24 Sep 2011 21:31:17 +0000 Received: from localhost (cpe-66-65-62-183.nyc.res.rr.com [66.65.62.183]) (authenticated bits=0) by shards.monkeyblade.net (8.14.4/8.14.4) with ESMTP id p8OLUvF8031898 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 24 Sep 2011 14:30:59 -0700 Date: Sat, 24 Sep 2011 23:33:00 -0000 Message-Id: <20110924.173057.89439041678554283.davem@davemloft.net> To: hp@bitrange.com Cc: gcc-patches@gcc.gnu.org, ebotcazou@adacore.com Subject: Re: [PATCH] Add VIS intrinsics header for sparc. From: David Miller In-Reply-To: References: <20110922.021541.2162102036631303082.davem@davemloft.net> <20110924.020832.1683526855969392801.davem@davemloft.net> Mime-Version: 1.0 Content-Type: Text/Plain; charset=us-ascii Content-Transfer-Encoding: 7bit X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2011-09/txt/msg01480.txt.bz2 From: Hans-Peter Nilsson 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_ (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.