[Patch?][RFC][RTL] clobber handling & buildin expansion - missing insn_invalid_p call [PR100418]

[Patch?][RFC][RTL] clobber handling & buildin expansion - missing insn_invalid_p call [PR100418]

[Tobias Burnus]

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

Hi Eric, hi all,

currently, gcn (amdgcn-amdhsa) bootstrapping fails as Alexandre's
patch to __builtin_memset (applied yesterday) now does more expansions.

The problem is [→ PR100418]
   (set(reg:DI)(plus:DI(reg:DI)(const_int)))  [= "adddi3"]
This fails with gcn as gcn has two clobbers for "adddi3" - and when
   expand_insn
is called, INSN_CODE == -1 via:
   icode = recog_memoized (insn);
alias
   INSN_CODE (insn) = recog (PATTERN (insn), insn, 0);
As the "int *pnum_clobber" argument is NULL (well, '0'), the
clobbers are not available - which causes the pattern fail.

I think that's a general issue with the RTX code generated by
builtins.c – except that most targets either do not
have clobbers for the used operators — or the code is by
chance fixed:

For instance, I see that several "if" blocks being processed in
recog.c's insn_invalid_p via 'cleanup_cfg (CLEANUP_NO_INSN_DEL)';
the innermost parts of the call chain are:
apply_change_group → verify_changes → insn_invalid_p

* * *

The attached patch seems to solve the GCN issue. Does it look OK?

Or does the insn_invalid_p call come too late?
If so, any suggestion where it would fit best?

Tobias,
who is more a FE and early-ME person.

-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank Thürauf

[-- Attachment #2: recog.diff --]
[-- Type: text/x-patch, Size: 541 bytes --]

extract_insn: Call insn_invalid_p is insn cannot be found.

gcc/ChangeLog:

	* recog.c (extract_insn): Call insn_invalid_p if
	recog_memoized did not find the insn.

diff --git a/gcc/recog.c b/gcc/recog.c
index eb617f11163..4ddc5d185af 100644
--- a/gcc/recog.c
+++ b/gcc/recog.c
@@ -2766,6 +2766,8 @@ extract_insn (rtx_insn *insn)
 	 and get the constraints.  */
 
       icode = recog_memoized (insn);
+      if (icode < 0 && !insn_invalid_p (insn, false))
+	icode = INSN_CODE (insn);
       if (icode < 0)
 	fatal_insn_not_found (insn);
 

Re: [Patch?][RFC][RTL] clobber handling & buildin expansion - missing insn_invalid_p call [PR100418]

[Jeff Law]

On 5/5/2021 7:50 AM, Tobias Burnus wrote:
> Hi Eric, hi all,
>
> currently, gcn (amdgcn-amdhsa) bootstrapping fails as Alexandre's
> patch to __builtin_memset (applied yesterday) now does more expansions.
>
> The problem is [→ PR100418]
>   (set(reg:DI)(plus:DI(reg:DI)(const_int)))  [= "adddi3"]
> This fails with gcn as gcn has two clobbers for "adddi3" - and when
>   expand_insn
> is called, INSN_CODE == -1 via:
>   icode = recog_memoized (insn);
> alias
>   INSN_CODE (insn) = recog (PATTERN (insn), insn, 0);
> As the "int *pnum_clobber" argument is NULL (well, '0'), the
> clobbers are not available - which causes the pattern fail.
>
> I think that's a general issue with the RTX code generated by
> builtins.c – except that most targets either do not
> have clobbers for the used operators — or the code is by
> chance fixed:
>
> For instance, I see that several "if" blocks being processed in
> recog.c's insn_invalid_p via 'cleanup_cfg (CLEANUP_NO_INSN_DEL)';
> the innermost parts of the call chain are:
> apply_change_group → verify_changes → insn_invalid_p
>
> * * *
>
> The attached patch seems to solve the GCN issue. Does it look OK?
>
> Or does the insn_invalid_p call come too late?
> If so, any suggestion where it would fit best?
>
> Tobias,
> who is more a FE and early-ME person.
>
> -----------------
> Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München 
> Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, 
> Frank Thürauf
>
> recog.diff
>
> extract_insn: Call insn_invalid_p is insn cannot be found.
>
> gcc/ChangeLog:
>
> 	* recog.c (extract_insn): Call insn_invalid_p if
> 	recog_memoized did not find the insn.
Was this issue on GCN fixed by Andrew/Jakub's change to use 
force_operand rather than emit_move_insn?

Jeff

Re: [Patch?][RFC][RTL] clobber handling & buildin expansion - missing insn_invalid_p call [PR100418]

[Andrew Stubbs]

On 30/05/2021 19:51, Jeff Law wrote:
> 
> 
> On 5/5/2021 7:50 AM, Tobias Burnus wrote:
>> Hi Eric, hi all,
>>
>> currently, gcn (amdgcn-amdhsa) bootstrapping fails as Alexandre's
>> patch to __builtin_memset (applied yesterday) now does more expansions.
>>
>> The problem is [→ PR100418]
>>   (set(reg:DI)(plus:DI(reg:DI)(const_int)))  [= "adddi3"]
>> This fails with gcn as gcn has two clobbers for "adddi3" - and when
>>   expand_insn
>> is called, INSN_CODE == -1 via:
>>   icode = recog_memoized (insn);
>> alias
>>   INSN_CODE (insn) = recog (PATTERN (insn), insn, 0);
>> As the "int *pnum_clobber" argument is NULL (well, '0'), the
>> clobbers are not available - which causes the pattern fail.
[...]
> Was this issue on GCN fixed by Andrew/Jakub's change to use 
> force_operand rather than emit_move_insn?

Yes, that's what that patch fixed.

Tobias's patch would also fix the issue, and any future issues with a 
similar problem.

Andrew