This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] vrp: fold ffs to ctz
Il 04/06/2012 11:31, Richard Guenther ha scritto:
> + val = compare_range_with_value (NE_EXPR, vr, integer_zero_node, &sop);
> + if (!val || !integer_onep (val))
> + return false;
>
> please add a value_range_nonzero_p helper alongside value_range_nonnegative_p.
>
> + fndecl = builtin_decl_implicit (target_builtin_code);
> + lhs = gimple_call_lhs (stmt);
> + gcc_assert (TREE_TYPE (TREE_TYPE (fndecl)) == TREE_TYPE (lhs));
>
> eh, if you care to check this please fail instead of ICEing ...
The verifier would fail very soon anyway. I might as well remove the
assertion altogether.
> Do we always have CTZ if we have FFS? Can't there be a target that
> implements FFS as opcode but not CTZ, so you'd slow down things?
> Thus, should the transform be conditonal on target support for CTZ
> or no target support for FFS?
Hmm, SH and (some semi-obscure variant of) SPARC. But actually SPARC
should define a clz pattern instead; SH should have a popcount pattern +
a generic trick to expand ctz/ffs in terms of popcount. I'll submit
those before applying this patch.
> Please add a comment to the code as to what transform you are doing
> here.
>
> + /* Convert argument type. */
> + argtype = TREE_VALUE (TYPE_ARG_TYPES (TREE_TYPE (fndecl)));
> + tem = create_tmp_reg (argtype, NULL);
> + newop = gimple_build_assign_with_ops (NOP_EXPR, tem, op0, NULL_TREE);
> + tem = make_ssa_name (tem, newop);
> + gimple_assign_set_lhs (newop, tem);
> + gsi_insert_before (gsi, newop, GSI_SAME_STMT);
>
> why is that necessary?
Argument checking for GIMPLE_CALL is almost nonexistent, but I would
like to be nice and create my calls with good arguments.
> Can you at least wrap it inside a
>
> if (!useless_type_conversion_p (argtype, TREE_TYPE (op0)))
>
> ?
Yes.
Thanks for the review!
Paolo