From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate.crashing.org (gate.crashing.org [63.228.1.57]) by sourceware.org (Postfix) with ESMTP id 1D0463858420 for ; Sun, 31 Oct 2021 03:25:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1D0463858420 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=kernel.crashing.org Received: from gate.crashing.org (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.14.1) with ESMTP id 19V3ObT4027166; Sat, 30 Oct 2021 22:24:37 -0500 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 19V3Ob2m027162; Sat, 30 Oct 2021 22:24:37 -0500 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Sat, 30 Oct 2021 22:24:36 -0500 From: Segher Boessenkool To: Bill Schmidt Cc: gcc-patches@gcc.gnu.org, dje.gcc@gmail.com Subject: Re: [PATCH 06/18] rs6000: Builtin expansion, part 1 Message-ID: <20211031032436.GD614@gate.crashing.org> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-3.2 required=5.0 tests=BAYES_00, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, KAM_NUMSUBJECT, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=no autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 31 Oct 2021 03:25:41 -0000 Hi! On Wed, Sep 01, 2021 at 11:13:42AM -0500, Bill Schmidt wrote: > Differences between the old and new support in this patch include: > - Make use of the new builtin data structures, directly looking up > a function's information rather than searching for the function > multiple times; Is that measurable, do you think? > - Test for enablement of builtins at expand time, to support #pragma > target changes within a compilation unit; But not within a function, right? > Note that these six patches must be pushed together, because otherwise > unused parameter warnings in the stub functions will prevent bootstrap. Must be *committed* as one commit, even. Committing them as six separate commits and pushing them all at once will do nothing for people who try to bisect, etc. Merging patches is easy with Git though :-) > +/* Expand ALTIVEC_BUILTIN_MASK_FOR_LOAD. */ > +rtx > +rs6000_expand_ldst_mask (rtx target, tree arg0) > + { > + return target; > + } Interesting leading spaces, heh. Please fix. > +/* Expand the HTM builtin in EXP and store the result in TARGET. */ > +static rtx > +new_htm_expand_builtin (bifdata *bifaddr, rs6000_gen_builtins fcode, > + tree exp, rtx target) > +{ > + return const0_rtx; > +} The function commment should say what the return value is. > +/* Expand an expression EXP that calls a built-in function, > + with result going to TARGET if that's convenient > + (and in mode MODE if that's convenient). > + SUBTARGET may be used as the target for computing one of EXP's operands. > + IGNORE is nonzero if the value is to be ignored. > + Use the new builtin infrastructure. */ > +static rtx > +rs6000_expand_new_builtin (tree exp, rtx target, > + rtx subtarget ATTRIBUTE_UNUSED, > + machine_mode ignore_mode ATTRIBUTE_UNUSED, > + int ignore ATTRIBUTE_UNUSED) Don't use ATTRIBUTE_UNUSED? We have C++ now, you can leave out the parameter name, with the same effect (other than it does not make you go blind ;-) ). In the case where you still want to show the name, you can do something like rs6000_expand_new_builtin (tree exp, trx target, rtx /*subtarget*/, machine_mode, int /*ignore*/) (There is no argument MODE btw, the comment needs some tweaking). > + /* We have two different modes (KFmode, TFmode) that are the IEEE > + 128-bit floating point type, depending on whether long double is the > + IBM extended double (KFmode) or long double is IEEE 128-bit (TFmode). KFmode *always* is IEEE QP. TFmode is the one that can be different. > + It is simpler if we only define one variant of the built-in function, > + and switch the code when defining it, rather than defining two built- > + ins and using the overload table in rs6000-c.c to switch between the > + two. If we don't have the proper assembler, don't do this switch > + because CODE_FOR_*kf* and CODE_FOR_*tf* will be CODE_FOR_nothing. */ > + if (FLOAT128_IEEE_P (TFmode)) > + switch (icode) > + { > + default: > + break; default: goes at the *end*. And you can usually leave it out. > + case CODE_FOR_sqrtkf2_odd: > + icode = CODE_FOR_sqrttf2_odd; > + break; So please do this the other way? In libgcc "tf" means double-double, it is historical. So let's do the clearer thing please: translate tf to kf in this handling (when tf *does* mean kf ;-) ) > + /* In case of "#pragma target" changes, we initialize all builtins > + but check for actual availability now, during expand time. For > + invalid builtins, generate a normal call. */ > + bifdata *bifaddr = &rs6000_builtin_info_x[uns_fcode]; > + bif_enable e = bifaddr->enable; > + > + if (e != ENB_ALWAYS > + && (e != ENB_P5 || !TARGET_POPCNTB) if (!(e == ENB_ALWAYS || (e == ENB_P5 && TARGET_POPCNTB) etc. Computers are better at De Morgan than humans are. It is much more important to write clear code. This often means using fewer negations. > + const int MAX_BUILTIN_ARGS = 6; > + tree arg[MAX_BUILTIN_ARGS]; > + rtx op[MAX_BUILTIN_ARGS]; > + machine_mode mode[MAX_BUILTIN_ARGS + 1]; Arrays are always better with a short comment. Why is the "mode" array one entry longer, btw? > + rtx pat; > + bool void_func = TREE_TYPE (TREE_TYPE (fndecl)) == void_type_node; > + int k; These things should be declared where they are first used. The initialisation of void_func should not be hidden amidst boring declarations. "k" needs a comment. > + if (!insn_data[icode].operand[i+k].mode) > + mode[i+k] = TARGET_64BIT ? Pmode : SImode; That is mode[i+k] = Pmode; always. Does this depend on VOIDmode being equal to 0? That is guaranteed, but if you write out VOIDmode elsewhere, do it here as well? > + else > + mode[i+k] = insn_data[icode].operand[i+k].mode; > + } So this is mode[i+k] = insn_data[icode].operand[i+k].mode; if (!mode[i+k]) mode[i+k] = Pmode; > + switch (bifaddr->restr[i]) > + { > + default: default: goes at the end. > + case RES_BITS: > + { > + size_t mask = (1 << bifaddr->restr_val1[i]) - 1; 1 is an int, it can overflow much before a size_t would. size_t mask = 1; mask <<= bifaddr->restr_val1[i]; mask--; > + tree restr_arg = arg[bifaddr->restr_opnd[i] - 1]; > + STRIP_NOPS (restr_arg); > + if (TREE_CODE (restr_arg) != INTEGER_CST > + || TREE_INT_CST_LOW (restr_arg) & ~mask) Manual De Morgan again? More later too, ugh. Well at least these are trivial either way. > + case RES_VAR_RANGE: > + { > + tree restr_arg = arg[bifaddr->restr_opnd[i] - 1]; > + STRIP_NOPS (restr_arg); > + if (TREE_CODE (restr_arg) == INTEGER_CST > + && !IN_RANGE (tree_to_shwi (restr_arg), > + bifaddr->restr_val1[i], > + bifaddr->restr_val2[i])) > + { > + error ("argument %d must be a variable or a literal " > + "between %d and %d, inclusive", > + bifaddr->restr_opnd[i], bifaddr->restr_val1[i], > + bifaddr->restr_val2[i]); > + return CONST0_RTX (mode[0]); > + } > + break; > + } This error check is incongruent with the rest, and with its error message? If it is not an INTEGER_CST, it does not check anything about it. That sounds like trouble later. > + if (fcode == RS6000_BIF_PACK_IF > + && TARGET_LONG_DOUBLE_128 && !TARGET_IEEEQUAD) Curious line breaks. Break before each &&, or don't break before the first one? > + { > + icode = CODE_FOR_packtf; > + fcode = RS6000_BIF_PACK_TF; The fcode was for IF, can you use TF now? > + uns_fcode = (size_t)fcode; > + } (space after cast) > + else if (fcode == RS6000_BIF_UNPACK_IF > + && TARGET_LONG_DOUBLE_128 && !TARGET_IEEEQUAD) > + { > + icode = CODE_FOR_unpacktf; > + fcode = RS6000_BIF_UNPACK_TF; > + uns_fcode = (size_t)fcode; > + } (same issues) > + switch (nargs) > + { > + default: Just Say No. Don't write 0 == err and don't put default: first. It does not improve your code. It makes it worse, instead. Yoda can understand Yoda-speak, it comes natural to him. Yoda is not amongst the readers of your code though, so please write in the common idiom :-) This is okay for trunk with these things fixed. Thanks! Segher