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] Fix incorrect optab expansion for ulcmp on ARM.


On Sat, Jan 28, 2006 at 07:00:22AM -0700, Roger Sayle wrote:
> 
> On Fri, 27 Jan 2006, Carlos O'Donell wrote:
> > 2006-01-27  Carlos O'Donell  <carlos@codesourcery.com>
> >
> > 	* optabs.c (prepare_cmp_insn): If unbaised and unsigned then bias
> > 	the comparison routine return.
> >
> > 	* gcc.dg/unsigned-long-compare.c: New test.
> 
> This is OK for both mainline and the 4.1 branch.

Excellent.

> I'm a bit uncomfortable that there's still something wrong with the
> design of this code.  compare_from_rtx's use of emit_cmp_insn only
> seems safe for cc0 targets.  Even with your fix to prepare_cmp_insn,
> we still ignore the change to *pmode with integer comparisons, and
> also ignore the changes in prepare_float_lib_cmp.  compare_from_rtx
> could be moved from do_jump.c to optabs.c and call directly to
> prepare_cmp_insn and emit_cmp_and_jump_insn_1, but how emit_cmp_insn
> is supposed to work is a mystery.

Yes, I agree it is a bit of a mystery. There are some inconsistencies in
the interface that deals with these parameters. As I say in the code
comment, the silent expectation is that the output comparison is
functionally equivalent to the input comparison.

> Despite my concerns that this papering over a fundamental design
> issue, your patch is an improvement of current mainline, it does
> resolve the regression in your testcase, and could only affect
> the ARM.  It undoubtedly the right thing to do for 4.1, but I'd
> appreciate it if we could investigate some follow-up improvements.

The follow-up improvements may take time to audit and discuss, they
would include some changes suggested by Daniel Jacobowitz which include
an audit of the callers for these functions.

> There may also be potential optimizer benefits of allowing
> prepare_cmp_insn to use equality/inequality comparisons instead
> of LT, GT, LTU, GTU so that the result value may be CSE'd into
> the following code.  i.e. "result == 2" has more information
> content than "result > 1" during the following RTL optimizers.

Yes, quite true.

> Thanks for investigating.  The new comment is particularly informative.

Thank you!

Cheers,
Carlos.
-- 
Carlos O'Donell
CodeSourcery
carlos@codesourcery.com
(650) 331-3385 x716


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