This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [Patch : H8300] Fix regressions for bit related instructions
- From: Jeff Law <law at redhat dot com>
- To: "Naveen H. S" <Naveen dot S at kpitcummins dot com>
- Cc: "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>, Prafulla Thakare <Prafulla dot Thakare at kpitcummins dot com>, Kaushik Phatak <Kaushik dot Phatak at kpitcummins dot com>
- Date: Tue, 23 Feb 2010 16:17:39 -0700
- Subject: Re: [Patch : H8300] Fix regressions for bit related instructions
- References: <371569CBCFB2E745B891DBB88B2DFDDD1987D25618@KCINPUNHJCMS01.kpit.com>
On 02/16/10 23:47, Naveen H. S wrote:
Hi Jeff,
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
Please find attached the "h8sx.patch" patch which fixes regressions
for H8SX and related targets.
The patch basically disallows use of register operands in bit patterns
for H8SX target. H8SX code results in ICE when trying to generate
SUBREG(gen_rtx_SUBREG) for bit patterns. Hence, bit patterns of H8SX
was restricted to use only memory mode. As H8SX target is a higher
version of H8300, it generates memory addressing RTL pattern for bit
patterns. Hence this approach would optimize the code in H8SX as it
reduces the overhead of loading registers before operating on bit
patterns.
We had already posted the initial patch at following link:-
http://gcc.gnu.org/ml/gcc-patches/2008-10/msg00674.html
However, we could not follow up on the patch due to some other
priorities. Please let us know whether it is the correct approach to
solve the ICE generated with H8SX target or let us where to modify the
code to fix these errors.
Ah. I recall that patch. My first reaction when it was posted in
2008 was that it was papering over a bug elsewhere. I didn't dive
further into it because I expected you to. I still feel that initial
review is correct.
I did build a cross compiler so that I could take a closer look and see
if there was anything obviously wrong which would ultimately lead to the
assertion failure in validate_subreg. I think the fundamental problem
here is someone didn't heed the comment at the start of h8300.md when SX
support was added and I probably missed that buglet when the SX code was
reviewed.
;; There's currently no way to have an insv/extzv expander for the H8/300H
;; because word_mode is different for the H8/300 and H8/300H.
That comment should include the H8/S and H8/SX everywhere H8/300H is
mentioned.
If we look at the expanders we have:
(define_expand "insv"
[(set (zero_extract:HI (match_operand:HI 0 "general_operand" "")
(match_operand:HI 1 "general_operand" "")
(match_operand:HI 2 "general_operand" ""))
(match_operand:HI 3 "general_operand" ""))]
"TARGET_H8300 || TARGET_H8300SX"
(define_expand "extzv"
[(set (match_operand:HI 0 "register_operand" "")
(zero_extract:HI (match_operand:HI 1 "bit_operand" "")
(match_operand:HI 2 "general_operand" "")
(match_operand:HI 3 "general_operand" "")))]
"TARGET_H8300 || TARGET_H8300SX"
The fundamental problem is the various operands on the insv/extv
patterns specify HImode. I believe at the time I wrote that comment
(1995?) the backend was required to specify modes for the insv/extv
operands in the expander -- omitting the modes in the pattern was not
supported. Thus there was no way to properly support insv/extv on the
H8/300H, H8/S or H8/SX targets.
The assertion failures you're seeing occur because we have an SImode
operand, but the extzv pattern requires a HImode operand. We try to
create a suitable SUBREG to get at the low word via this code:
/* If op0 is a register, we need it in EXT_MODE to make it
acceptable to the format of ext(z)v. */
if (REG_P (xop0) && GET_MODE (xop0) != ext_mode)
xop0 = gen_rtx_SUBREG (ext_mode, xop0, 0);
Where ext_mode will be HImode and xop0 will be an SImode object.
This isn't going to work on a big endian target. That code should
probably be
xop0 = gen_lowpart_SUBREG (ext_mode, xop0);
Similarly for the code in store_bit_field_1.
I *think* that's enough to fix this testcase and I think it's correct
for other targets as well. I'm playing around with that change right now.
Long term, the right thing to do is remove the modes on the operands of
the extv/insv expanders and change the expander's condition to allow it
to be used on all the H8 targets (insv/extv operands are assumed to be
WORD_MODE if they are not explicitly specified). This may require
twiddling matching patterns or creating new patterns.
If you could also verify that a copyright assignment is in place for:
KPIT has signed copyright assignment and some of our patches are already
accepted and present in current GCC sources. Please let us know whether
we should provide the signed copyright assignment approval mail.
Thanks. I figured that was the case, I just wanted to be sure.
jeff