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
