[PATCH,H8300] Fix ICE due to insv and extzv patterns

[PATCH,H8300] Fix ICE due to insv and extzv patterns

[Naveen H.S.]

[-- Attachment #1: Type: text/plain, Size: 822 bytes --]

Hi,

In H8SX target, there are many regression failures(ICE) due to the
"insv" and "extzv" patterns. Please find attached the patch "h8sx.patch"
that allows only memory operands in the "loc" field of extract pattern
for H8SX target. Similarly register and sub register operands are 
allowed in the "loc" field of extract pattern for other targets. 
Hence, the patch fixes many regressions for H8SX target in C and C++
test suite.

Regression tested on H8300 targets.

2008-10-16  Naveen.H.S  <naveen.hs@kpitcummins.com>
            
	* config/h8300/predicates.md (bit_reg_operand): New predicate.
	* config/h8300/h8300.md (insv): Modify the predicate in "loc" 
	field.
	* config/h8300/h8300.md (insv): Likewise.

Regards,
Naveen.H.S.
KPIT Cummins Infosystems Ltd,
Pune (INDIA)
www.kpitgnutools.com

[-- Attachment #2: h8sx.patch --]
[-- Type: application/octet-stream, Size: 1484 bytes --]

--- /gcc/config/h8300/h8300.md	2008-02-21 03:00:35.000000000 +0530
+++ /gcc/config/h8300/h8300.md	2008-10-15 08:03:17.000000000 +0530
@@ -3222,7 +3222,7 @@
    (set_attr "length" "8,6")])
 
 (define_expand "insv"
-  [(set (zero_extract:HI (match_operand:HI 0 "general_operand" "")
+  [(set (zero_extract:HI (match_operand:HI 0 "bit_reg_operand" "")
 			 (match_operand:HI 1 "general_operand" "")
 			 (match_operand:HI 2 "general_operand" ""))
 	(match_operand:HI 3 "general_operand" ""))]
@@ -3298,7 +3298,7 @@
 
 (define_expand "extzv"
   [(set (match_operand:HI 0 "register_operand" "")
-	(zero_extract:HI (match_operand:HI 1 "bit_operand" "")
+	(zero_extract:HI (match_operand:HI 1 "bit_reg_operand" "")
 			 (match_operand:HI 2 "general_operand" "")
 			 (match_operand:HI 3 "general_operand" "")))]
   "TARGET_H8300 || TARGET_H8300SX"
--- /gcc/config/h8300/predicates.md	2007-08-02 16:19:31.000000000 +0530
+++ /gcc/config/h8300/predicates.md	2008-10-15 08:03:51.000000000 +0530
@@ -480,3 +480,24 @@
 
   return (code == IOR || code == XOR);
 })
+
+;; Return nonzero if OP is a MEM suitable for bit manipulation insns in
+;; H8SX target and register operand in other targets.
+
+(define_predicate "bit_reg_operand"
+  (match_code "mem,reg")
+{
+  if (TARGET_H8300SX)
+    {
+  return (GET_CODE (op) == MEM
+          && OK_FOR_U (op));
+    }
+  if (!TARGET_H8300SX)
+    {
+  if (GET_CODE (op) == REG)
+    return 1;
+  if (GET_CODE (op) == SUBREG)
+    return 1;
+    }
+})
+

Re: [PATCH,H8300] Fix ICE due to insv and extzv patterns

[Jeff Law]

Naveen H.S. wrote:
> Hi,
>
> In H8SX target, there are many regression failures(ICE) due to the
> "insv" and "extzv" patterns. Please find attached the patch "h8sx.patch"
> that allows only memory operands in the "loc" field of extract pattern
> for H8SX target. Similarly register and sub register operands are 
> allowed in the "loc" field of extract pattern for other targets. 
> Hence, the patch fixes many regressions for H8SX target in C and C++
> test suite.
>
> Regression tested on H8300 targets.
>
> 2008-10-16  Naveen.H.S  <naveen.hs@kpitcummins.com>
>             
> 	* config/h8300/predicates.md (bit_reg_operand): New predicate.
> 	* config/h8300/h8300.md (insv): Modify the predicate in "loc" 
> 	field.
> 	* config/h8300/h8300.md (insv): Likewise.
>   
This doesn't make sense.  Isn't the H8SX a superset of the original 
H8/300 part?  It sounds like you're papering over a problem elsewhere.

Can you elaborate on precisely why you want to restrict the H8SX 
insv/extv expanders to only working on memory operands?  ie, *why* do 
you think this patch is the correct way to fix this problem?

jeff

RE: [PATCH,H8300] Fix ICE due to insv and extzv patterns

[Naveen H.S.]

Hi Jeff,

>> Isn't the H8SX a superset of the original H8/300 part?

Yes, H8SX is a superset of H8/300 family.

>> Can you elaborate on precisely why you want to restrict the H8SX 
>> insv/extv expanders to only working on memory operands? 

insv/extv expanders in H8300 allow memory operands only for H8SX. When
a reg/subreg is used for H8SX in these expanders, it generates ICE in
(gen_rtx_SUBREG).

Hence only memory operands are allowed for H8SX in the patch. The 
present modification fixes around 500 regression failures(ICE) in C 
testsuite and around 50 regression failures(ICE) in C++ testsuite.

>> why do you think this patch is the correct way to fix this problem?

It was observed that allowing reg/subreg for H8SX in these expanders
was the cause of ICE. Hence only memory operands were allowed for H8SX
in the expanders. If the assumption is wrong, please suggest a solution
that would help to me solve ICE in a better way.

Regards,
Naveen.H.S.
KPIT Cummins Infosystems Ltd,
Pune (INDIA)
www.kpitgnutools.com

Re: [PATCH,H8300] Fix ICE due to insv and extzv patterns

[Jeff Law]

Naveen H.S. wrote:
> insv/extv expanders in H8300 allow memory operands only for H8SX. When
> a reg/subreg is used for H8SX in these expanders, it generates ICE in
> (gen_rtx_SUBREG).
>
> Hence only memory operands are allowed for H8SX in the patch. The 
> present modification fixes around 500 regression failures(ICE) in C 
> testsuite and around 50 regression failures(ICE) in C++ testsuite.
>   
But it still sounds like you're just papering over the problem.  There's 
nothing which would prevent the same problem from occurring with the 
original H8 port, or more specifically you haven't told me why this 
scenario can't happen on the original H8 port.

Furthermore, you haven't described the conditions leading to the abort 
in enough detail for me to understand precisely why you're getting an  
ICE.  So I can't even start to guess what an alternative solution might be.

Jeff

RE: [PATCH,H8300] Fix ICE due to insv and extzv patterns

[Naveen H.S.]

Hi Jeff,

>> There's nothing which would prevent the same problem from occurring 
>> with the original H8 port

Yes, this seems to be a generic problem in H8 target. However, the 
original H8 port does not seem to generate the RTL patterns for insv
and extzv expanders. Hence, it does not result in ICE. 

H8SX is an advanced port in H8 target and generates the most optimized
code. So register and memory operands are allowed only in H8SX target
for insv and extzv expanders in their "loc" field unlike the original
H8 port which allows only register operands. If memory operands are
used in the "loc" field of H8SX target, it would generate more 
optimised code as it reduces the redundant instructions of loading and
storing the values between register and memory. Hence, the patch allows
only memory operands in the "loc" field of H8SX target. 

>> you haven't described the conditions leading to the abort in enough
>> detail for me to understand precisely why you're getting an ICE. 

The error generated while compiling the testcase was "internal compiler
error: in gen_rtx_SUBREG, at emit-rtl.c". On further investigation, it
led to the following line:-
gcc_assert (validate_subreg (mode, GET_MODE (reg), reg, offset));
Hence, it was assumed that the error was due to generating subreg and 
only memory operands were allowed in the "loc" field of insv and extzv 
expanders for H8SX. 

Regards,
Naveen.H.S.
KPIT Cummins Infosystems Ltd,
Pune (INDIA)
www.kpitgnutools.com

Re: [PATCH,H8300] Fix ICE due to insv and extzv patterns

[Jeff Law]

Naveen H.S. wrote:
> H8SX is an advanced port in H8 target and generates the most optimized
> code. So register and memory operands are allowed only in H8SX target
> for insv and extzv expanders in their "loc" field unlike the original
> H8 port which allows only register operands. If memory operands are
> used in the "loc" field of H8SX target, it would generate more 
> optimised code as it reduces the redundant instructions of loading and
> storing the values between register and memory. Hence, the patch allows
> only memory operands in the "loc" field of H8SX target. 
>
>   
>>> you haven't described the conditions leading to the abort in enough
>>> detail for me to understand precisely why you're getting an ICE. 
>>>       
>
> The error generated while compiling the testcase was "internal compiler
> error: in gen_rtx_SUBREG, at emit-rtl.c". On further investigation, it
> led to the following line:-
> gcc_assert (validate_subreg (mode, GET_MODE (reg), reg, offset));
> Hence, it was assumed that the error was due to generating subreg and 
> only memory operands were allowed in the "loc" field of insv and extzv 
> expanders for H8SX. 
>   
There's still nowhere near enough information here.  I'm sorry,but I'm 
going to have to reject the patch until you provide a detailed analysis 
of the problem and your solution. 

jeff