[Bug rtl-optimization/95267] New: [ICE][gcse]: in process_insert_insn at gcse.c

[Bug rtl-optimization/95267] New: [ICE][gcse]: in process_insert_insn at gcse.c

[xuemaosheng at huawei dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95267

            Bug ID: 95267
           Summary: [ICE][gcse]: in process_insert_insn at gcse.c
           Product: gcc
           Version: 7.3.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: rtl-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: xuemaosheng at huawei dot com
  Target Milestone: ---

In rtl-optimization gcse pass produces a invalid insn, and finally in function 
process_insert_insn reach the code gcc_unreachable ().

static rtx_insn *
process_insert_insn (struct gcse_expr *expr)
{
  ......
  else
    {
      rtx_insn *insn = emit_insn (gen_rtx_SET (reg, exp));

      if (insn_invalid_p (insn, false))
        gcc_unreachable ();
    }

  .....
}

function backtrace is shown as the following:
0xa127cc process_insert_insn
        /gcc/gcse.c:2074
0xa12cce pre_edge_insert
        gcc/gcse.c:2244
0xa137ee pre_gcse
        /gcc/gcse.c:2636
0xa1394f one_pre_gcse_pass
        /gcc/gcse.c:2691
0xa169cd execute_rtl_pre
        /gcc/gcse.c:4097
0xa16a80 execute
        /gcc/gcse.c:4141

the invalid insn produced by gcse pass is shown as the following:
(insn 423 0 0 (set (reg:V8HF16 442 [ userDagc ])
        (unspec:V8HF16 [
                (reg:V8HF16 171 [ _54 ])
                (reg/v:BF8 235 [ boolRsrp ])
                (reg/v:V8HF16 234 [ userDagc ])
            ] UNSPEC_MOVTVFM)) -1
     (nil))

while the original valid insn is shown as the followig:
(insn 261 262 263 30 (set (reg/v:V8HF16 236 [ userDagc ])
        (unspec:V8HF16 [
                (reg:V8HF16 171 [ _54 ])
                (reg/v:BF8 235 [ boolRsrp ])
                (reg/v:V8HF16 236 [ userDagc ])
            ] UNSPEC_MOVTVFM))
"/home2/w00364751/L1Rat/UBBP/src/RatV2/L1Uudp/Srs/nMM/MeasProc/Scheduler/ProcSchedule/src/SRS_UserProc_FDDMM_Private.c":1156
433 {movtv8hf16}
     (expr_list:REG_EQUAL (unspec:V8HF16 [
                (reg:V8HF16 171 [ _54 ])
                (reg/v:BF8 235 [ boolRsrp ])
                (reg/v:V8HF16 234 [ userDagc ])
            ] UNSPEC_MOVTVFM)
        (nil)))

After our analysis, we think that the source of invalid insn is the function
can_assign_to_reg_without_clobbers_p.
function backtrace is shown as the following:
#0  can_assign_to_reg_without_clobbers_p (x=0x7ffff7234460, mode=V8HF16mode) at
/gcc/gcse.c:882
#1  0x0000000000a0fe9e in want_to_gcse_p (x=0x7ffff7234460, mode=V8HF16mode,
max_distance_ptr=0x0) at /gcc/gcse.c:819
#2  0x0000000000a10a71 in hash_scan_set (set=0x7ffff71fe910,
insn=0x7ffff71ff370, table=0x1c39980 <expr_hash_table>)
    at /gcc/gcse.c:1260
#3  0x0000000000a10dbc in hash_scan_insn (insn=0x7ffff71ff370, table=0x1c39980
<expr_hash_table>) at /gcc/gcse.c:1370
#4  0x0000000000a11947 in compute_hash_table_work (table=0x1c39980
<expr_hash_table>) at /gcc/gcse.c:1625
#5  0x0000000000a11a95 in compute_hash_table (table=0x1c39980
<expr_hash_table>) at /gcc/gcse.c:1674
#6  0x0000000000a138de in one_pre_gcse_pass () at 
/gcc/gcse.c:2680
#7  0x0000000000a169ce in execute_rtl_pre () at 
/gcc/gcse.c:4097

the function can_assign_to_reg_without_clobbers_p return true if we can assign
X to a pseudo register of mode MODE such that the resulting insn does not
result in clobbering a hard register as a side-effect.

In our target, the insn is valid since operands[0] is inout reg, this means
that
the insn 262 not only define reg/v:V8HF16 236, but also use reg/v:V8HF16 236.
(insn 261 262 263 30 (set (reg/v:V8HF16 236 [ userDagc ])
        (unspec:V8HF16 [
                (reg:V8HF16 171 [ _54 ])
                (reg/v:BF8 235 [ boolRsrp ])
                (reg/v:V8HF16 236 [ userDagc ])
            ] UNSPEC_MOVTVFM)) 
the REG_EQUAL is
     (expr_list:REG_EQUAL (unspec:V8HF16 [
                (reg:V8HF16 171 [ _54 ])
                (reg/v:BF8 235 [ boolRsrp ])
                (reg/v:V8HF16 234 [ userDagc ])
            ] UNSPEC_MOVTVFM)
        (nil)))

the function can_assign_to_reg_without_clobbers_p use TEST_INSN to judge the
REG_EQUAL is valid or not.
the SET_DEST of TEST_INSN use the regno of FIRST_PSEUDO_REGISTER * 2;
if out target FIRST_PSEUDO_REGISTER is 117, which means that
FIRST_PSEUDO_REGISTER * 2 = 234.
the TEST_ISNN became the following code.
(insn 63 0 0 (set (reg:V8HF16 234)
        (unspec:V8HF16 [
                (reg:V8HF16 171 [ _54 ])
                (reg/v:BF8 235 [ boolRsrp ])
                (reg/v:V8HF16 234 [ userDagc ])
            ] UNSPEC_MOVTVFM)) -1
     (nil))

the insn looks like a valid insn.  However, this is a coincidence，because
FIRST_PSEUDO_REGISTER * 2 = 234， regno 234 is the same as the use reg
reg/v:V8HF16 234, the insn output operands[0],looks like a inout reg.
If  FIRST_PSEUDO_REGISTER is 100 in my target, then the pattern would become
(insn 63 0 0 (set (reg:V8HF16 200)
        (unspec:V8HF16 [
                (reg:V8HF16 171 [ _54 ])
                (reg/v:BF8 235 [ boolRsrp ])
                (reg/v:V8HF16 234 [ userDagc ])
            ] UNSPEC_MOVTVFM)) -1
     (nil))

Then the pattern would be invalid.

bool
can_assign_to_reg_without_clobbers_p (rtx x, machine_mode mode)
{
  ......

  /* Otherwise, check if we can make a valid insn from it.  First initialize
     our test insn if we haven't already.  */
  if (test_insn == 0)
    {
      test_insn
        = make_insn_raw (gen_rtx_SET (gen_rtx_REG (word_mode,
                                                   FIRST_PSEUDO_REGISTER * 2),
                                      const0_rtx));
      SET_NEXT_INSN (test_insn) = SET_PREV_INSN (test_insn) = 0;
      INSN_LOCATION (test_insn) = UNKNOWN_LOCATION;
    }

  /* Now make an insn like the one we would make when GCSE'ing and see if
     valid.  */
  PUT_MODE (SET_DEST (PATTERN (test_insn)), mode);
  SET_SRC (PATTERN (test_insn)) = x;

  icode = recog (PATTERN (test_insn), test_insn, &num_clobbers);

  ......

  return can_assign;
}


So in my opinion, the TEST_INSN use a stable regno FIRST_PSEUDO_REGISTER * 2 in
some case would make some invalid pattern looks like a valid pattern, and
finally will cause internal compiler error.

So can we initialize the SET_DEST every times rather than only initialize once 
And use max_reg_num to replace FIRST_PSEUDO_REGISTER * 2 ?

@@ -851,15 +851,12 @@ can_assign_to_reg_without_clobbers_p (rtx x, machine_mode
mode)

   /* Otherwise, check if we can make a valid insn from it.  First initialize
      our test insn if we haven't already.  */
-  if (test_insn == 0)
-    {
-      test_insn
-       = make_insn_raw (gen_rtx_SET (gen_rtx_REG (word_mode,
-                                                  FIRST_PSEUDO_REGISTER * 2),
-                                     const0_rtx));
-      SET_NEXT_INSN (test_insn) = SET_PREV_INSN (test_insn) = 0;
-      INSN_LOCATION (test_insn) = UNKNOWN_LOCATION;
-    }
+  test_insn = NULL;
+  test_insn
+    = make_insn_raw (gen_rtx_SET (gen_rtx_REG (word_mode, max_reg_num ()),
+                                  const0_rtx));
+  SET_NEXT_INSN (test_insn) = SET_PREV_INSN (test_insn) = 0;
+  INSN_LOCATION (test_insn) = UNKNOWN_LOCATION;

[Bug rtl-optimization/95267] [ICE][gcse]: in process_insert_insn at gcse.c

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95267

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |WAITING
   Last reconfirmed|                            |2020-05-22

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I think this is a back-end issue.
Can you provide the definition of movtv8hf16 ?
I don't think you can do:
(set (match_operand 0 predicate constraint)
        (unspec:V8HF16 [
                (match_operand 1 predicate constraint)
                (match_operand 2 predicate constraint)
                (match_dup 0)
            ] UNSPEC_MOVTVFM)))

Rather you need to do:
(set (match_operand 0 predicate constraint)
        (unspec:V8HF16 [
                (match_operand 1 predicate constraint)
                (match_operand 2 predicate constraint)
                (match_operand 3 predicate "0")
            ] UNSPEC_MOVTVFM)))

[Bug rtl-optimization/95267] [ICE][gcse]: in process_insert_insn at gcse.c

[xuemaosheng at huawei dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95267

--- Comment #2 from otcmaf <xuemaosheng at huawei dot com> ---
(In reply to Andrew Pinski from comment #1)
> I think this is a back-end issue.
> Can you provide the definition of movtv8hf16 ?
> I don't think you can do:
> (set (match_operand 0 predicate constraint)
>         (unspec:V8HF16 [
>                 (match_operand 1 predicate constraint)
>                 (match_operand 2 predicate constraint)
>                 (match_dup 0)
>             ] UNSPEC_MOVTVFM)))
> 
> Rather you need to do:
> (set (match_operand 0 predicate constraint)
>         (unspec:V8HF16 [
>                 (match_operand 1 predicate constraint)
>                 (match_operand 2 predicate constraint)
>                 (match_operand 3 predicate "0")
>             ] UNSPEC_MOVTVFM)))

we use this definition of movtv8hf16:
(define_insn "movtv8hf16"                                                       
  [(set (match_operand:V8HF16 0 "register_operand_s" "+Zrv")                    
        (unspec:V8HF16 [(match_operand:V8HF16 1 "register_operand_s" "Zrv")     
                        (match_operand:BF8 2 "register_operand_s" "Zrb")        
                        (match_dup 0)]UNSPEC_MOVTVFM))]

why we can't use (match_dup 0) ?

[Bug rtl-optimization/95267] [ICE][gcse]: in process_insert_insn at gcse.c

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95267

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |RESOLVED
         Resolution|---                         |INVALID

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
The internals documentation documents this even, read:
https://gcc.gnu.org/onlinedocs/gccint/RTL-Template.html#index-match_005fdup

>From that:
Note that match_dup should not be used to tell the compiler that a particular
register is being used for two operands (example: add that adds one register to
another; the second register is both an input operand and the output operand).
Use a matching constraint (see Simple Constraints) for those.

[Bug rtl-optimization/95267] [ICE][gcse]: in process_insert_insn at gcse.c

[xuemaosheng at huawei dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95267

--- Comment #4 from otcmaf <xuemaosheng at huawei dot com> ---
(In reply to Andrew Pinski from comment #3)
> The internals documentation documents this even, read:
> https://gcc.gnu.org/onlinedocs/gccint/RTL-Template.html#index-match_005fdup
> 
> From that:
> Note that match_dup should not be used to tell the compiler that a
> particular register is being used for two operands (example: add that adds
> one register to another; the second register is both an input operand and
> the output operand). Use a matching constraint (see Simple Constraints) for
> those.

However, in gcc/config/aarch64/aarch64-simd.md also has the similar usage。
For example：
1227 (define_insn "aarch64_simd_move_hi_quad_<mode>"                            
1228   [(set (match_operand:VQ 0 "register_operand" "+w,w")                     
1229         (vec_concat:VQ                                                     
1230           (vec_select:<VHALF>                                              
1231                 (match_dup 0)                                              
1232                 (match_operand:VQ 2 "vect_par_cnst_lo_half" ""))           
1233           (match_operand:<VHALF> 1 "register_operand" "w,r")))]            
1234   "TARGET_SIMD && !BYTES_BIG_ENDIAN"                                       
1235   "@                                                                       
1236    ins\\t%0.d[1], %1.d[0]                                                  
1237    ins\\t%0.d[1], %1"                                                      
1238   [(set_attr "type" "neon_ins")]                                           
1239 )     
the operands[0] is a inout reg, and the pattern also use (match_dup 0).

Another case in gcc/config/c6x/c6x.md
 440 (define_insn "*movstricthi_high"                                           
 441   [(set (match_operand:SI 0 "register_operand" "+ab")                      
 442         (ior:SI (and:SI (match_dup 0) (const_int 65535))                   
 443                 (ashift:SI (match_operand:SI 1 "const_int_operand" "IuB")  
 444                            (const_int 16))))]                              
 445   "reload_completed"                                                       
 446   "%|%.\\tmvklh\\t%$\\t%1, %0"                                             
 447   [(set_attr "units" "s")]) 

2930 (define_insn "setup_dsbt"                                                  
2931   [(set (match_operand:SI 0 "pic_register_operand" "+Z")                   
2932         (unspec:SI [(match_dup 0)                                          
2933                     (match_operand:SI 1 "symbolic_operand" "")]            
2934                    UNSPEC_SETUP_DSBT))]                                    
2935   "TARGET_DSBT"                                                            
2936   "%|%.\\tldw\\t%$\\t*+%0($DSBT_index%1), %0"                              
2937   [(set_attr "type" "load")                                                
2938    (set_attr "units" "d_addr")                                             
2939    (set_attr "dest_regfile" "b")                                           
2940    (set_attr "addr_regfile" "b")]) 


Do you mean that those pattern above are also wrong pattern ?
As you say that, that match_dup should not be used to tell the compiler that a
particular register is being used for two operands.
But I still think that the function can_assign_to_reg_without_clobbers_p can do
bettter, use max_reg_num to replace FIRST_PSEUDO_REGISTER * 2 and update
TEST_INSN when calling this function.

[Bug rtl-optimization/95267] [ICE][gcse]: in process_insert_insn at gcse.c

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95267

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to otcmaf from comment #4)
> Do you mean that those pattern above are also wrong pattern ?

YES those are broken.

[Bug rtl-optimization/95267] [ICE][gcse]: in process_insert_insn at gcse.c

[zhongyunde at tom dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95267

zhongyunde at tom dot com <zhongyunde at tom dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |zhongyunde at tom dot com

--- Comment #6 from zhongyunde at tom dot com <zhongyunde at tom dot com> ---
*** Bug 95210 has been marked as a duplicate of this bug. ***

[Bug rtl-optimization/95267] [ICE][gcse]: in process_insert_insn at gcse.c

[xuemaosheng at huawei dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95267

--- Comment #7 from otcmaf <xuemaosheng at huawei dot com> ---
(In reply to Andrew Pinski from comment #5)
> (In reply to otcmaf from comment #4)
> > Do you mean that those pattern above are also wrong pattern ?
> 
> YES those are broken.

Ok, thanks very much.