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 : H8300] Fix regressions for bit related instructions


On 01/21/10 05:20, Kaushik Phatak wrote:
Hi,

The following patch generates bit instructions for logical operations
on the H8 target. It also generates instructions in #xx:3 and #xx:4
immediate addressing modes for H8SX target.

The patch fixes 272 regressions on the H8SX target and overall it
fixes 525 regressions.
The original patch was earlier posted in the following Bugzilla link,
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38091

A few issues that came up have been addressed in this version of the
patch.

The patch "h8300_bit_insn.patch" is attached.

Regards,
Kaushik Phatak
www.kpitgnutools.com
This is basically OK, though we are currently in regression bugfixes only mode (where a regression is defined as something that previously worked, but no longer works). It's not clear to me how this is fixing any regressions since it appears to be mostly an optimization patch. If there is a hunk that is directly responsible for fixing bugs, we can break that out into its own patch that could be installed now, but I simply don't see which it would be.

In OK_FOR_Y2, you're missing a space after the exact_log2 call ie, it currently looks like

exact_log2(INTVAL (...)

It should look like

exact_log2 (INTVAL (...)

SImilarly for OK_FOR_Y0, except it should look like

exact_log2 (~INTVAL (...)

For andqi3_1, iorqi3_1 and xorqi3_1, you reversed the constraints (and the output templates). Presumably this was so you'd match the bit version before the more general logical instruction, right? Any plans to update the length computation for the bit variant of those insns -- it's always reporting 8 when we know it's often going to be shorter.

Why the MEM_VOLATILE_P in the conditions for bsethi_msx and its associated splitter?

You need a comment before the bit_register_indirect_operand predicate. Particularly since accepts (mem (const_int)) which to someone not intimately familiar with this port would seem odd.

If you could fix those issues and attach the patch to PR 41998 I'll install the updated patch once gcc-4.5 branches and the mainline opens again for development work. If you could also verify that a copyright assignment is in place for:

Deepen Mantri & Sushil Kothawade since I believe they contributed to earlier versions of this patch and those contributions are large enough to require a copyright assignment. Alternatively, if kpit has a blanket assignment on file with the FSF for GCC changes, that would be sufficient to satisfy the copyright assignment requirements.

jeff


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