Re: [PATCH] PR libgcc/59714 complex division is surprising on aarch64

[Joseph Myers <joseph@codesourcery.com>]

On Thu, 25 Jan 2018, vladimir.mezentsev@oracle.com wrote:

> From: Vladimir Mezentsev <vladimir.mezentsev@oracle.com>
> 
> Tested on aarch64-linux-gnu, x86_64-pc-linux-gnu and sparc64-unknown-linux-gnu.
> No regression. New tests now passed.
> There is a performance degradation for complex double type:

This is, of course, something to consider for GCC 9; it's not suitable for 
the current development stage.

Could you provide a more extended writeup of the issue (starting with the 
argument for it being a bug at all), the approach you took and the 
rationale for the approach, when you resubmit the patch for GCC 9 stage 1?

> * libgcc/config/sparc/sfp-machine.h: New

Are you introducing a requirement for all architectures to provide 
sfp-machine.h?  If so, did you determine that SPARC was the only one 
lacking it?  If not the only one lacking it, you need to make sure that 
you do not break any existing architecture in GCC.

What about architectures using non-IEEE formats?  soft-fp is not suitable 
for those, but you mustn't break them.  Remember that e.g. TFmode can be 
IBM long double, and other ?Fmode similarly need not be IEEE; the name 
only indicates how many bytes are in the format, nothing else about it.

What about powerpc __divkc3?

What was the rationale for using soft-fp rather than adding appropriate 
built-in functions as suggested in a comment?

These are the sorts of issues you need to cover in your patch write-up, to 
demonstrate that you have allowed for the range of targets supported in 
GCC, have taken care to avoid breaking them and have made deliberate 
choices that are appropriate in each case (but might not be the same for 
every target).

> +static inline int get_cde(int c, int d)

Lots of the new functions need comments on them to explain their 
semantics.  Also, the formatting needs fixing to follow the GNU Coding 
Standards ("static inline int" on its own line, space before '(').

> +  if (cde != 0) {

Similarly, bad formatting, "{" goes on the next line.

-- 
Joseph S. Myers
joseph@codesourcery.com