This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] [4.3. projects] Vectorize int to float conversions
On 1/25/07, Tehila Meyzels <TEHILA@il.ibm.com> wrote:
> Here's a more thorough review.
>
> + /* Returns a code for builtin that realizes vectorized version of
> + int to float conversion, or NULL_TREE if not available. */
> + tree (* builtin_intfloat_conversion) (tree);
> +
>
> I don't like the name - maybe builtin_vectorized_conversion?
OK. No problem.
>
> + operation = GIMPLE_STMT_OPERAND (stmt, 1);
> + code = TREE_CODE (operation);
> + if (code != FIX_TRUNC_EXPR && code != FLOAT_EXPR)
> + return false;
>
> why place this restriction here? I think this should be done in the
> target hook.
Cause I wanted to take it one step at a time, and make it explicit what
this function is meant to support for now. Also - do we really want to
invoke the builtin function on any opcode? I agree that we may want to
extend the list of conversion opcodes we handle, but I still think it
should be checked here before we call the target builtin - the idea is that
the target hook is expected to handle a certain closed set of conversion
tree-codes, right?
Yes, sort of. But the target hook can also return NULL_TREE if it doesn't
support the conversion. So, I'm fine to keep the check in the generic code
if you add it to the rs6000 target hook as well.
>
> + /* Check types of lhs and rhs. */
> + op0 = TREE_OPERAND (operation, 0);
> + rhs_type = TREE_TYPE (op0);
>
> so you only allow unary operations - please add a helper function here
> that extracts the source type from the operation so we can also allow
> function calls here.
>
> + /* FORNOW: need to extend to support short<->float conversions
> as well. */
> + if (nunits_out != nunits_in)
> + return false;
>
> I have been thinking about this myself - there is high-level support
> for this missing,
Not sure what high-level support you are referring to...
There didn't seem to be a way to do for example V2DFmode -> V2DImode
conversion in two steps (there's no cvtpd2di but only cvtsd2di). The closest
I could find would be enhancing the pattern recognition code to emit more
than one instruction.
> so this is ok for now. But the canonical way is to write ??? or FIXME
> instead of
> FORNOW.
>
> + /* Check that types are both integral or non-integral. */
> + if ((INTEGRAL_TYPE_P (rhs_type) && INTEGRAL_TYPE_P (lhs_type))
> + || (!INTEGRAL_TYPE_P (rhs_type) && !INTEGRAL_TYPE_P (lhs_type)))
> + return false;
>
> huh? isn't it the point to have rhs_type real and lhs_type int or the
> other way around?
>
Exactly. That's why we return 'false' if lhs and rhs are both integral or
both floats.
Ah, so I was confused by the comment which suggests you are checking for
the opposite. Maybe change it to
/* Bail out if the types are both integral or non-integral. */
> + /* Check ... */
> + ncopies = LOOP_VINFO_VECT_FACTOR (loop_vinfo) / nunits_in;
> + gcc_assert (ncopies >= 1);
>
> Check what? Why with an assert?
Oops, wrong comment, will fix that.
It is supposed to say "Sanity check: make sure that at least one copy of
the vectorized stmt needs to be generated".
Otherwise, it means that the VF is say 8, while the nunits of the stmt in
question is, say, 16, which is not expected
to happen. The vectorizer is supposed to determine the VF according to the
smallest type in the loop (i.e. largest nunits)).
This is the reason for the 'assert'.
Ok.
>
> + /* Supportable by target? */
> + if (!targetm.vectorize.builtin_intfloat_conversion
> + || !targetm.vectorize.builtin_intfloat_conversion (vectype_in))
> + return false;
>
> I see you are passing the _type_ to the target hook. You should pass the
> whole operation there to allow vectorizing for example different rounding
> modes.
Can you please send me some examples for what you have in mind?
(Do you mean lrint family, that rounds vs. (int), that truncates, for
example?)
Below I changed my mind somewhat ;) I was indeed thinking of function
calls, but this is probably better handled by calling to the
builtin_vectorized_function
target hook (which can be added later). Still I'd like you to pass
the TREE_CODE
of the operation to the hook so the target has a chance to reject an operation
it cannot handle (we at least have FLOAT_EXPR and FIX_TRUNC_EXPR).
>
> + prev_stmt_info = NULL;
> + for (j = 0; j < ncopies; j++)
> + {
> + vec_oprnd0 = vect_get_vec_def_for_operand (op0, stmt, NULL);
> + params = build_tree_list (NULL_TREE, vec_oprnd0);
> +
> + builtin_decl =
> + targetm.vectorize.builtin_intfloat_conversion (vectype_in);
>
> Wrong indentation, and no need to call this over and over again - cache
it
> from the invocation above where you check for target support.
Sure, I'll fix that.
BTW - I think I've followed build_vectorized_function_call code, so I
think we have the same problem there, no? ;-)
From a quick look I cannot see it there, but you may be right.
>
> + new_stmt = build_function_call_expr (builtin_decl, params);
> +
> + /* Arguments are ready. create the new vector stmt. */
> + new_stmt = build2 (GIMPLE_MODIFY_STMT, void_type_node, vec_dest,
> + new_stmt);
> + new_temp = make_ssa_name (vec_dest, new_stmt);
> + GIMPLE_STMT_OPERAND (new_stmt, 0) = new_temp;
> + vect_finish_stmt_generation (stmt, new_stmt, bsi);
> +
> + if (j == 0)
> + STMT_VINFO_VEC_STMT (stmt_info) = *vec_stmt = new_stmt;
> + else
> + STMT_VINFO_RELATED_STMT (prev_stmt_info) = new_stmt;
> + prev_stmt_info = vinfo_for_stmt (new_stmt);
> + }
> + return true;
> + }
>
> Do you have a testcase excercising ncopies != 1?
Sure, I'll add one.
Thanks!
>
> *************** vect_transform_stmt (tree stmt, block_st
> *** 3873,3878 ****
> --- 4009,4019 ----
> gcc_assert (done);
> break;
>
> + case type_int_float_vec_info_type:
> + done = vectorizable_int_float_conversion (stmt, bsi, &vec_stmt);
> + gcc_assert (done);
> + break;
> +
>
> all the names involving *_int_float_* should probably just be *_*, i.e.
> vectorizable_conversion, etc.
OK.
>
> *************** expand_vector_operations_1 (block_stmt_i
> *** 405,411 ****
> && TREE_CODE_CLASS (code) != tcc_binary)
> return;
>
> ! if (code == NOP_EXPR || code == VIEW_CONVERT_EXPR)
> return;
>
> gcc_assert (code != CONVERT_EXPR);
> --- 405,413 ----
> && TREE_CODE_CLASS (code) != tcc_binary)
> return;
>
> ! if (code == NOP_EXPR
> ! || code == FLOAT_EXPR
> ! || code == VIEW_CONVERT_EXPR)
> return;
>
> There's a lot missing here, CONVERT_EXPR and FIX_TRUNC_EXPR come to
> my mind.
I agree.
FIX_TRUNC_EXPR - sure, as I wrote in my previous note (I missed it).
CONVERT_EXPR - is this the tree code in lrint cases?
No, CONVERT_EXPR is just a synonym for NOP_EXPR really.
I haven't found anything more that seems relevant, beside these 2, in
tree.def.
Yes, for int <-> float conversion we only have FIX_TRUNC_EXPR and FLOAT_EXPR,
all others (like lrint) are represented using calls to builtin functions.
>
> Note that the patch somewhat overlaps with the support for vectorizing
builtin
> functions, so you might want to call the builtin_vectorized_function
> target hook
> for conversions that involve a function call. This also suggests that
maybe
> the conversion facility should use another generic
> builtin_vectorized_operation
> target hook which you would pass the tree_code and the vector type
> (as the builtin_vectorized_function hook gets the builtin function id and
the
> result vector type). To ever support nunits_in != nunits_out natively it
> would be nice to also pass the desired operand vector type to the
> target hooks.
>
> So it would look like
>
> tree (* builtin_vectorized_operation) (unsigned tree_code, tree
> lhs_vec_type, tree rhs_vec_type);
>
> Can you rework the patch with this suggested interface change? We can
add
> the support for type changing functions later if you like.
Sure.
Thanks for the helpful comments.
Tehila.
>
> Thanks,
> Richard.