This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]