[PATCH v5] Practical Improvement to libgcc Complex Divide

[Patrick McGehearty]

Thank you for your rapid feedback.
I'll fix the various formatting issues (spaces in the wrong places
and such as well as revise the Changelog magic) in the next submission.
It will wait for Joseph's review to also make any changes he suggests.
I'll also try to train myself to be more sensitive to gcc formatting
conventions while proofreading.

I'm reluctant to change or use XALLOCAVEC instead of alloca as that
is not the current style elsewhere in the routine.

I agree that namelen=strlen(name) would be an apparent optimization,
but since *name is declared const char, I would think the compiler
would need to compute strlen(name) only one time. Again, I'm reluctant
to change the existing code patterns.

On the strcpy, strncpy, and memcpy question, given short length of
the string being copied, I don't think it makes much difference.
The two other copy operations in the file are memcpy.
memcpy might be slightly better since it is generally more frequently
seen and more likely that gcc has special case code to inline
short fixed length memcpy as a few assignments. Even if both strncpy
and memcpy are inlined, the memcpy code may be simplier as it does
not need to be concerned with special treatment of nulls.
I'll change the strncpy to memcpy.

- patrick



On 12/8/2020 5:31 PM, Jakub Jelinek wrote:
> On Tue, Dec 08, 2020 at 10:32:33PM +0000, Patrick McGehearty via Gcc-patches wrote:
>> 2020-12-08 Patrick McGehearty <patrick.mcgehearty@oracle.com>
>>
>> * gcc/c-family/c-cppbuiltin.c - Add supporting macros for new complex divide.
>> * libgcc2.c (__divsc3, __divdc3, __divxc3, __divtc3): Improve complex divide.
>> * libgcc/config/rs6000/_divkc3.c - Complex divide changes for rs6000.
>> * gcc/testsuite/gcc.c-torture/execute/ieee/cdivchkd.c - double cdiv test.
>> * gcc/testsuite/gcc.c-torture/execute/ieee/cdivchkf.c - float cdiv test.
>> * gcc/testsuite/gcc.c-torture/execute/ieee/cdivchkld.c - long double cdiv test.
> Thanks for working on this, I'll defer review to Joseph, just want to add a few
> random comments.
> The above ChangeLog will not get through the commit checking scripts,
> one needs two spaces before and after name instead of just one,
> pathnames should be relative to the corresponding ChangeLog file and one
> should separate what goes to each ChangeLog, and lines except the first one
> should be tab indented.  So it should look like:
>
> 2020-12-08  Patrick McGehearty  <patrick.mcgehearty@oracle.com>
>
> gcc/c-family/
> 	* c-cppbuiltin.c (c_cpp_builtins): Add supporting macros for new
> 	complex divide.
> libgcc/
> 	* libgcc2.c (XMTYPE, XCTYPE, RBIG, RMIN, RMIN2, RMINSCAL, RMAX2):
> 	Define.
> 	(__divsc3, __divdc3, __divxc3, __divtc3): Improve complex divide.
> 	* config/rs6000/_divkc3.c (RBIG, RMIN, RMIN2, RMINSCAL, RMAX2):
> 	Define.
> 	(__divkc3): Improve complex divide.
> gcc/testsuite/
> 	* gcc.c-torture/execute/ieee/cdivchkd.c: New test.
> 	* gcc.c-torture/execute/ieee/cdivchkf.c: New test.
> 	* gcc.c-torture/execute/ieee/cdivchkld.c: New test.
>
> or so.
>
>> --- a/gcc/c-family/c-cppbuiltin.c
>> +++ b/gcc/c-family/c-cppbuiltin.c
>> @@ -1347,6 +1347,47 @@ c_cpp_builtins (cpp_reader *pfile)
>>   						  "PRECISION__"));
>>   	  sprintf (macro_name, "__LIBGCC_%s_EXCESS_PRECISION__", name);
>>   	  builtin_define_with_int_value (macro_name, excess_precision);
>> +
>> +	  if ((mode == TYPE_MODE (float_type_node))
>> +	      || (mode == TYPE_MODE (double_type_node))
>> +	      || (mode == TYPE_MODE (long_double_type_node)))
>> +	    {
>> +	      char val_name[64];
>> +	      char fname[8] = "";
>> +	      if (mode == TYPE_MODE (float_type_node))
>> +		strncpy(fname, "FLT",4);
> Formatting, there should be space before ( for calls, and space in between
> , and 4.  Also, what is the point of using strncpy?  strcpy or
> memcpy would do.
>
>> +	      else if (mode == TYPE_MODE (double_type_node))
>> +		strncpy(fname, "DBL",4);
>> +	      else if (mode == TYPE_MODE (long_double_type_node))
>> +		strncpy(fname, "LDBL",5);
>> +
>> +	      if ( (mode == TYPE_MODE (float_type_node))
>> +		   || (mode == TYPE_MODE (double_type_node)) )
> Formatting, no spaces in between the ( ( and ) ).
>> +		{
>> +		  macro_name = (char *) alloca (strlen (name)
>> +						+ sizeof ("__LIBGCC_EPSILON__"
>> +							  ));
> This should use XALLOCAVEC macro, so
> 		  macro_name
> 		    = XALLOCAVEC (char, strlen (name)
> 					+ sizeof ("__LIBGCC_EPSILON__"));
>
> I admit it is a preexisting problem in the code above it too.
>
>> +		  sprintf (macro_name, "__LIBGCC_%s_EPSILON__", name);
>> +		  sprintf( val_name, "__%s_EPSILON__", fname);
> Space before ( rather than after it.
>
>> +		  builtin_define_with_value (macro_name, val_name, 0);
>> +		}
>> +
>> +	      macro_name = (char *) alloca (strlen (name)
>> +					    + sizeof ("__LIBGCC_MAX__"));
> Again, XALLOCAVEC.  You could have remembered strlen (name) in a temporary
> when you use it multiple times.  Again it is used in the code earlier
> multiple times too and could be just remembered there.  GCC strlen
> pass can optimize some cases of using multiple strlen calls on the same
> string, but if there are intervening calls that could in theory change the
> string lengths it needs to recompute those.
> So just size_t name_len = stlren (name); and using name_len would be
> IMHO better.
>
>> +	      sprintf (macro_name, "__LIBGCC_%s_MAX__", name);
>> +	      sprintf( val_name, "__%s_MAX__", fname);
>> +	      builtin_define_with_value (macro_name, val_name, 0);
>> +
>> +	      macro_name = (char *) alloca (strlen (name)
>> +					    + sizeof ("__LIBGCC_MIN__"));
>> +	      sprintf (macro_name, "__LIBGCC_%s_MIN__", name);
>> +	      sprintf( val_name, "__%s_MIN__", fname);
>> +	      builtin_define_with_value (macro_name, val_name, 0);
>> +	    }
>> +#ifdef HAVE_adddf3
>> +	  builtin_define_with_int_value ("__LIBGCC_HAVE_HWDBL__",
>> +					 HAVE_adddf3);
>> +#endif
> 	Jakub
>