Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Jeff Law <law at redhat dot com>]

On 11/25/2016 05:18 AM, Tamar Christina wrote:
> Hi Joseph,
>
> I have updated the patch with the changes,
> w.r.t to the formatting, there are tabs there that seem to be rendered
> at 4 spaces wide. In my editor setup at 8 spaces they are correct.
Various random comments, mostly formatting.  I do think we're going to 
need another iteration.  The good news is I don't expect we'll be asking 
for major changes in direction, just trying to tie up various loose 
ends, so it shouldn't take nearly as long.

On a high level, presumably there's no real value in keeping the old 
code to "fold" fpclassify.  By exposing those operations as integer 
logicals for the fast path, if the FP value becomes a constant during 
the optimization pipeline we'll see the reinterpreted values flowing 
into the new integer logical tests and they'll simplify just like 
anything else.  Right?

The old IBM format is still supported, though they are expected to be 
moveing towards a standard ieee 128 bit format.  So my only concern is 
that we preserve correct behavior for those cases -- I don't really care 
about optimizing them.  So I think you need to keep them.

For documenting builtins, using existing builtins as a template.



> @@ -822,6 +882,736 @@ lower_builtin_setjmp (gimple_stmt_iterator *gsi)
>    gsi_remove (gsi, false);
>  }
>
> +static tree
> +emit_tree_and_return_var (gimple_seq *seq, tree arg)
Needs a function comment.

> +{
> +  if (TREE_CODE (arg) == SSA_NAME || VAR_P (arg))
> +    return arg;
> +
> +  tree tmp = create_tmp_reg (TREE_TYPE(arg));
Nit.  Need space between TREE_TYPE and the open-parn for its arglist

> +  gassign *stm = gimple_build_assign(tmp, arg);
Similarly between gimple_build_assign and its arglist.  This formatting 
nit is repeated often.  Please check the patch as a whole and correct. 
Probably the best way to go is a search for [a-z] immediately preceding 
an open paren.


> +/* This function builds an if statement that ends up using explicit branches
> +   instead of becoming a csel.  This function assumes you will fall through to
> +   the next statements after this condition for the false branch.  */
A function comment is supposed to document each argument (and the return 
value if any).  It's probably better to avoid referring to an ARM 
instruction (csel) and instead describe the intent more genericly.


> +static void
> +emit_tree_cond (gimple_seq *seq, tree result_variable, tree exit_label,
> +		tree cond, tree true_branch)
> +{
> +    /* Create labels for fall through.  */
> +  tree true_label = create_artificial_label (UNKNOWN_LOCATION);
Comment is indented too deep.  It should line up with the code.


> +
> +static tree
> +get_num_as_int (gimple_seq *seq, tree arg, location_t loc)
Needs function comment.  I'm not going to call out each one.  Please 
verify that every new function has a comment and that they document 
their arguments and return values.

Here's an example from builtins.c you might want to refer back to:

> /* For a memory reference expression EXP compute values M and N such that M
>    divides (&EXP - N) and such that N < M.  If these numbers can be determined,
>    store M in alignp and N in *BITPOSP and return true.  Otherwise return false
>    and store BITS_PER_UNIT to *alignp and any bit-offset to *bitposp.  */
>
> bool
> get_object_alignment_1 (tree exp, unsigned int *alignp,
>                         unsigned HOST_WIDE_INT *bitposp)
> {
>   return get_object_alignment_2 (exp, alignp, bitposp, false);
> }



> +  /* Check if the number that is being classified is close enough to IEEE 754
> +     format to be able to go in the early exit code.  */
> +static bool
> +use_ieee_int_mode (tree arg)
Comment should describe the argument.  Comment is indented 2 spaces too 
deep on both lines.  This occurs in several please.  I won't call each 
one out, but please walk through the patch as a whole and fix them.


> +{
> +  tree type = TREE_TYPE (arg);
> +
> +  machine_mode mode = TYPE_MODE (type);
> +
> +  const real_format *format = REAL_MODE_FORMAT (mode);
> +  const HOST_WIDE_INT type_width = TYPE_PRECISION (type);
> +  return (format->is_binary_ieee_compatible
> +	  && FLOAT_WORDS_BIG_ENDIAN == WORDS_BIG_ENDIAN
> +	  /* We explicitly disable quad float support on 32 bit systems.  */
> +	  && !(UNITS_PER_WORD == 4 && type_width == 128)
> +	  && targetm.scalar_mode_supported_p (mode));
> +}
Presumably this is why you needed the target.h inclusion.

Note that on some systems we even disable 64bit floating point support. 
I suspect this check needs a little re-thinking as I don't think that 
checking for a specific UNITS_PER_WORD is correct, nor is checking the 
width of the type.  I'm not offhand sure what the test should be, just 
that I think we need something better here.



> +
> +static tree
> +is_normal (gimple_seq *seq, tree arg, location_t loc)
> +{
> +  tree type = TREE_TYPE (arg);
> +
> +  machine_mode mode = TYPE_MODE (type);
> +  const real_format *format = REAL_MODE_FORMAT (mode);
> +  const tree bool_type = boolean_type_node;
> +
> +  /* Perform IBM extended format fixups if required.  */
> +  bool is_ibm_extended = perform_ibm_extended_fixups (&arg, &mode, &type, loc);
> +
> +  /* If not using optimized route then exit early.  */
> +  if (!use_ieee_int_mode (arg))
> +  {
Please check on your formatting.  The open-curly is indented two spaces 
relative to the IF statement.   I see multiple occurrences, so please go 
through the patch and check for them.  I'm not going to try and call out 
each one.






> +
> +static tree
> +is_infinity (gimple_seq *seq, tree arg, location_t loc)
There's a huge amount of code duplication between is_infinity and 
is_finite.  Would it make sense to try and refactor a bit to avoid the 
duplication?  Some of the early setup is even common among most (all?) 
if the is_* functions, consider factoring the common bits into routines 
you can reuse.


> +
> +/* Determines if the given number is a NaN value.
> +   This function is the last in the chain and only has to
> +   check if it's preconditions are true.  */
> +static tree
> +is_nan (gimple_seq *seq, tree arg, location_t loc)
So in the old code we checked UNGT_EXPR, in the new code's slow path you 
check UNORDERED.  Was that change intentional?

In general, I'm going to assume you've got the right tests.   I've done 
light spot checking and will probably do even more on the next iteration.

I see a lot of formatting and lack-of-function comment issues.  I'm 
comfortable with the overall direction though.  I'll want to look more 
closely at the helpers you're using to build up gimple code and how 
they're used.  I get the sense that we ought to already have something 
to do those things, and if not we may want to move yours into a more 
generic location.  I'll probably focus on that and trying to pick up any 
missed nits on the next iteration.  Again, the good news is that I don't 
expect the next iteration to take as long to get to.

Thanks for your patience.

jeff