Bug 112560 - [14 Regression] ICE in try_combine on pr112494.c
Summary: [14 Regression] ICE in try_combine on pr112494.c
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: rtl-optimization (show other bugs)
Version: 14.0
: P1 normal
Target Milestone: 14.0
Assignee: Uroš Bizjak
URL:
Keywords: ice-checking, ice-on-valid-code, patch
Depends on: 112494
Blocks:
  Show dependency treegraph
 
Reported: 2023-11-16 07:23 UTC by Jakub Jelinek
Modified: 2024-04-11 06:33 UTC (History)
4 users (show)

See Also:
Host:
Target: x86_64-linux-gnu
Build:
Known to work:
Known to fail:
Last reconfirmed: 2023-12-21 00:00:00


Attachments
Proposed patch (443 bytes, patch)
2023-11-28 17:33 UTC, Uroš Bizjak
Details | Diff
Proposed v2 patch (822 bytes, patch)
2024-03-11 11:54 UTC, Uroš Bizjak
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jakub Jelinek 2023-11-16 07:23:40 UTC
+++ This bug was initially created as a clone of Bug #112494 +++

As written in https://gcc.gnu.org/pipermail/gcc-patches/2023-November/636503.html
I see with --enable-checking=yes,rtl,extra on x86_64-linux:

Executing on host: /home/jakub/src/gcc/obj48/gcc/xgcc -B/home/jakub/src/gcc/obj48/gcc/  /home/jakub/src/gcc/gcc/testsuite/gcc.target/i386/pr112494.c    -fdiagnostics-plain-output   -Og -fno-tree-copy-prop -fno-tree-fre -fno-tree-ccp -fno-tree-forwprop -S -o pr112494.s    (timeout = 300)
spawn -ignore SIGHUP /home/jakub/src/gcc/obj48/gcc/xgcc -B/home/jakub/src/gcc/obj48/gcc/ /home/jakub/src/gcc/gcc/testsuite/gcc.target/i386/pr112494.c -fdiagnostics-plain-output -Og -fno-tree-copy-prop -fno-tree-fre -fno-tree-ccp -fno-tree-forwprop -S -o pr112494.s
during RTL pass: combine
/home/jakub/src/gcc/gcc/testsuite/gcc.target/i386/pr112494.c: In function 'main':
/home/jakub/src/gcc/gcc/testsuite/gcc.target/i386/pr112494.c:17:1: internal compiler error: RTL check: expected elt 0 type 'e' or 'u', have 'E' (rtx unspec) in try_combine, at combine.cc:3237
0x834954 rtl_check_failed_type2(rtx_def const*, int, int, int, char const*, int, char const*)
        ../../gcc/rtl.cc:761
0xd8a46e try_combine
        ../../gcc/combine.cc:3237
0x25d65ff combine_instructions
        ../../gcc/combine.cc:1264
0x25d65ff rest_of_handle_combine
        ../../gcc/combine.cc:15080
0x25d65ff execute
        ../../gcc/combine.cc:15124
Please submit a full bug report, with preprocessed source (by using -freport-bug).
Please include the complete backtrace with any bug report.
See <https://gcc.gnu.org/bugs/> for instructions.
compiler exited with status 1

This is on
3236			  /* Just replace the CC reg with a new mode.  */
3237			  SUBST (XEXP (*cc_use_loc, 0), newpat_dest);
3238			  undobuf.other_insn = cc_use_insn;
in combine.cc, where *cc_use_loc is
(unspec:DI [
        (reg:CC 17 flags)
    ] UNSPEC_PUSHFL)
on which XEXP (guess combine assumes CC must be used inside of a
comparison).
Comment 1 Drea Pinski 2023-11-18 06:17:12 UTC
This code seems to originally was added with https://gcc.gnu.org/pipermail/gcc-patches/2011-April/311365.html 

The discussion on the patch continues into May:
https://gcc.gnu.org/pipermail/gcc-patches/2011-May/312415.html
Comment 2 Drea Pinski 2023-11-18 06:17:26 UTC
Confirmed.
Comment 3 Uroš Bizjak 2023-11-28 17:33:18 UTC
Created attachment 56705 [details]
Proposed patch

The code assumes that cc_use_loc represents a comparison operator. Skip the modification of CC-using operation if this is not the case.
Comment 4 Uroš Bizjak 2023-11-29 09:37:46 UTC
Patch at [1].

[1] https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638589.html
Comment 5 Uroš Bizjak 2024-03-11 11:54:51 UTC
Created attachment 57666 [details]
Proposed v2 patch

New version of patch in testing.

This version handles change of the mode of CC reg outside comparison. Now we scan the RTX and change the mode of the CC reg at the proper place. We are guaranteed by find_single_use that cc_use_loc can be non-NULL only when exactly one user follows the combined comparison.

In case of unsupported cc_use_insn combine will be undone. To avoid combine failure, pushfl<mode>2 in i386.md is changed to accept all MODE_CC modes.
Comment 6 Uroš Bizjak 2024-03-12 17:54:59 UTC
v3 patch at [1]

[1] https://gcc.gnu.org/pipermail/gcc-patches/2024-March/647634.html
Comment 7 GCC Commits 2024-04-08 20:22:50 UTC
The master branch has been updated by Uros Bizjak <uros@gcc.gnu.org>:

https://gcc.gnu.org/g:eaccdba315b86d374a4e72b9dd8fefb0fc3cc5ee

commit r14-9847-geaccdba315b86d374a4e72b9dd8fefb0fc3cc5ee
Author: Uros Bizjak <ubizjak@gmail.com>
Date:   Mon Apr 8 20:54:30 2024 +0200

    combine: Fix ICE in try_combine on pr112494.c [PR112560]
    
    The compiler, configured with --enable-checking=yes,rtl,extra ICEs with:
    
    internal compiler error: RTL check: expected elt 0 type 'e' or 'u', have 'E' (rtx unspec) in try_combine, at combine.cc:3237
    
    This is
    
    3236                      /* Just replace the CC reg with a new mode.  */
    3237                      SUBST (XEXP (*cc_use_loc, 0), newpat_dest);
    3238                      undobuf.other_insn = cc_use_insn;
    
    in combine.cc, where *cc_use_loc is
    
    (unspec:DI [
            (reg:CC 17 flags)
        ] UNSPEC_PUSHFL)
    
    combine assumes CC must be used inside of a comparison and uses XEXP (..., 0)
    without checking on the RTX type of the argument.
    
    Replace cc_use_loc with the entire new RTX only in case cc_use_loc satisfies
    COMPARISON_P predicate.  Otherwise scan the entire cc_use_loc RTX for CC reg
    to be updated with a new mode.
    
            PR rtl-optimization/112560
    
    gcc/ChangeLog:
    
            * combine.cc (try_combine): Replace cc_use_loc with the entire
            new RTX only in case cc_use_loc satisfies COMPARISON_P predicate.
            Otherwise scan the entire cc_use_loc RTX for CC reg to be updated
            with a new mode.
            * config/i386/i386.md (@pushf<mode>2): Allow all CC modes for
            operand 1.
Comment 8 Richard Biener 2024-04-09 08:26:37 UTC
Fixed I suppose.
Comment 9 Uroš Bizjak 2024-04-09 08:28:38 UTC
(In reply to Richard Biener from comment #8)
> Fixed I suppose.

Yes - I plan backport the patch to at least gcc-13.
Comment 10 Segher Boessenkool 2024-04-10 17:55:46 UTC
It is still wrong.  You're trying to sweep tour wrong assumptions under the rug,
but they will only rear up elsewhere.  Just fix the actual *target* problem!
Comment 11 Uroš Bizjak 2024-04-10 18:34:29 UTC
(In reply to Segher Boessenkool from comment #10)
> It is still wrong.  You're trying to sweep tour wrong assumptions under the
> rug,
> but they will only rear up elsewhere.  Just fix the actual *target* problem!

I can't see what could be wrong with:

(define_insn "@pushfl<mode>2"
  [(set (match_operand:W 0 "push_operand" "=<")
	(unspec:W [(match_operand 1 "flags_reg_operand")]
		  UNSPEC_PUSHFL))]
  "GET_MODE_CLASS (GET_MODE (operands[1])) == MODE_CC"
  "pushf{<imodesuffix>}"
  [(set_attr "type" "push")
   (set_attr "mode" "<MODE>")])

it is just a push of the flags reg to the stack. If the push can't be
described in this way, then it is the middle end at fault, we can't
just change modes at will.
Comment 12 Segher Boessenkool 2024-04-10 22:44:34 UTC
You cannot use a :CC value as argument of an unspec, as explained before.

The result of a comparison is expressed as a comparison, in RTL.  This patch
allows malformed RTL in more places than before, not progress at all.
Comment 13 Uroš Bizjak 2024-04-11 06:33:16 UTC
(In reply to Segher Boessenkool from comment #12)
> You cannot use a :CC value as argument of an unspec, as explained before.
> 
> The result of a comparison is expressed as a comparison, in RTL.  This patch
> allows malformed RTL in more places than before, not progress at all.

During our discussion we determined that this form with UNSPEC is actually a copy operation, so it is not an use [1] of CC register, because "use" is in the form of cc-compared-to-0.

[1] https://gcc.gnu.org/pipermail/gcc-patches/2024-March/647426.html

Now, let's see the part my patch fixes. The original change that introduced the functionality (See Comment #1) updates the "use" of the CC register. It assumes exclusively cc-compared-to-0 use, but there are several patterns in various target .md files that use naked CC register. The "???" comment suggests that the transformation tripped on this and thus the "verify the zero_rtx" was bolted on. The zero_rtx is inherent part of the regular CC reg "use", so this addition _mostly_ weeded out invalid access with naked CC reg.

If the CC reg is used as the source of copy operation ("move"), with or without UNSPEC, then the unpatched compiler will blindly use:

SUBST (XEXP (*cc_use_loc, 0), newpat_dest);

which *assumes* the certain form of the changed expression. Failed assumption will lead to memory corruption, and this is what my patch prevents.

Patched compiler will find the sole use of the naked CC reg (due to find_single_use) in the RTX, and update its mode at the right place. If the new mode is not recognized by the insn pattern, then the combination is rejected.

In any case, we are trading silent memory corruption with failed combine attempt. In my rule book, this is "progress" with bold, capital letters.