[Bug target/104335] New: [12 regression] build failure if go is included in languages after r12-6747

[Bug target/104335] New: [12 regression] build failure if go is included in languages after r12-6747

[seurer at gcc dot gnu.org]

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

            Bug ID: 104335
           Summary: [12 regression] build failure if go is included in
                    languages after r12-6747
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: seurer at gcc dot gnu.org
  Target Milestone: ---

g:aa8cfe785953a0e87d2472311e1260cd98c605c0, r12-6747 

This is a non-bootstrap build on a power 10 system with the configuration: 
configure --enable-languages=all --with-cpu=power10 --disable-bootstrap 


libtool: compile:  /home/seurer/gcc/git/build/gcc-test/./gcc/gccgo
-B/home/seurer/gcc/git/build/gcc-test/./gcc/
-B/home/seurer/gcc/git/install/gcc-test/powerpc64le-unknown-linux-gnu/bin/
-B/home/seurer/gcc/git/install/gcc-test/powerpc64le-unknown-linux-gnu/lib/
-isystem
/home/seurer/gcc/git/install/gcc-test/powerpc64le-unknown-linux-gnu/include
-isystem
/home/seurer/gcc/git/install/gcc-test/powerpc64le-unknown-linux-gnu/sys-include
-O2 -g -I . -c -fgo-pkgpath=golang.org/x/tools/internal/analysisinternal
/home/seurer/gcc/git/gcc-test/libgo/go/golang.org/x/tools/internal/analysisinternal/analysis.go
-o golang.org/x/tools/internal/analysisinternal.o
during RTL pass: ce2
/home/seurer/gcc/git/gcc-test/libgo/go/golang.org/x/tools/internal/analysisinternal/analysis.go:
In function 'golang.org/x/tools/internal/analysisinternal.FindBestMatch':
/home/seurer/gcc/git/gcc-test/libgo/go/golang.org/x/tools/internal/analysisinternal/analysis.go:402:1:
internal compiler error: in validate_condition_mode, at
config/rs6000/rs6000.cc:11358
  402 | func FindBestMatch(pattern string, idents []*ast.Ident) ast.Expr {
      | ^
0x11128e5f validate_condition_mode(rtx_code, machine_mode)
        /home/seurer/gcc/git/gcc-test/gcc/config/rs6000/rs6000.cc:11354
0x1112909b rs6000_generate_compare
        /home/seurer/gcc/git/gcc-test/gcc/config/rs6000/rs6000.cc:15153
0x11132f63 rs6000_emit_int_cmove(rtx_def*, rtx_def*, rtx_def*, rtx_def*)
        /home/seurer/gcc/git/gcc-test/gcc/config/rs6000/rs6000.cc:16320
0x11840903 gen_movsicc(rtx_def*, rtx_def*, rtx_def*, rtx_def*)
        /home/seurer/gcc/git/gcc-test/gcc/config/rs6000/rs6000.md:5365
0x10a678e7 rtx_insn* insn_gen_fn::operator()<rtx_def*, rtx_def*, rtx_def*,
rtx_def*>(rtx_def*, rtx_def*, rtx_def*, rtx_def*) const
        /home/seurer/gcc/git/gcc-test/gcc/recog.h:407
0x10a678e7 maybe_gen_insn(insn_code, unsigned int, expand_operand*)
        /home/seurer/gcc/git/gcc-test/gcc/optabs.cc:7962
0x10a68a93 maybe_expand_insn(insn_code, unsigned int, expand_operand*)
        /home/seurer/gcc/git/gcc-test/gcc/optabs.cc:7993
0x10a68a93 emit_conditional_move_1
        /home/seurer/gcc/git/gcc-test/gcc/optabs.cc:5030
0x10a68a93 emit_conditional_move_1
        /home/seurer/gcc/git/gcc-test/gcc/optabs.cc:4991
0x10a68bdb emit_conditional_move(rtx_def*, rtx_def*, rtx_def*, rtx_def*,
rtx_def*, machine_mode)
        /home/seurer/gcc/git/gcc-test/gcc/optabs.cc:4980
0x11b9408f noce_emit_cmove
        /home/seurer/gcc/git/gcc-test/gcc/ifcvt.cc:1753
0x11b94d5b try_emit_cmove_seq
        /home/seurer/gcc/git/gcc-test/gcc/ifcvt.cc:3179
0x11b9c0a7 noce_convert_multiple_sets
        /home/seurer/gcc/git/gcc-test/gcc/ifcvt.cc:3394
0x11b9d8bf noce_process_if_block
        /home/seurer/gcc/git/gcc-test/gcc/ifcvt.cc:3613
0x11ba08bf noce_find_if_block
        /home/seurer/gcc/git/gcc-test/gcc/ifcvt.cc:4367
0x11ba08bf find_if_header
        /home/seurer/gcc/git/gcc-test/gcc/ifcvt.cc:4572
0x11ba08bf if_convert
        /home/seurer/gcc/git/gcc-test/gcc/ifcvt.cc:5713
0x11ba1a0b execute
        /home/seurer/gcc/git/gcc-test/gcc/ifcvt.cc:5866


commit e9ebb86799fd77cdd5351078230c114a90e66066 (HEAD)
Author: Robin Dapp <rdapp@linux.ibm.com>
Date:   Wed Nov 27 13:53:40 2019 +0100

    ifcvt/optabs: Allow using a CC comparison for emit_conditional_move.

[Bug target/104335] [12 regression] build failure if go is included in languages after r12-6747

[wschmidt at gcc dot gnu.org]

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

Bill Schmidt <wschmidt at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |12.0
           Keywords|                            |build, ice-on-valid-code

[Bug target/104335] [12 regression] build failure if go is included in languages after r12-6747

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1

[Bug target/104335] [12 regression] build failure if go is included in languages after r12-6747

[rdapp at linux dot ibm.com]

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

rdapp at linux dot ibm.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rdapp at linux dot ibm.com

--- Comment #1 from rdapp at linux dot ibm.com ---
The problem is that we pass a CC comparison to rs6000_emit_cmove ().  In the
sequence before it will return false via

  if (FLOAT_MODE_P (compare_mode) && !FLOAT_MODE_P (result_mode))

because compare_mode == SFmode and result_mode == DImode.

Now it continues with compare_mode == CCFPmode and result_mode still DImode. 
Finally, in rs6000_generate_compare the validation fails.

It looks like this function does not expect CC comparison.  It looks like you
always try to generate one in the .md file already, regardless of whether the
incoming comparison already is a CC comparison or not.

I'm not really sure how to proceed.  I guess CCFPmode is not a real FLOAT_MODE
but maybe we should not continue at this point anyway?  Or would we need some
other mechanism now to make backends aware that this situation can occur now? 
I haven't checked all of them but I was expecting the backend to just return
NULL_RTX if it cannot handle a specific combination of ops.

[Bug target/104335] [12 regression] build failure if go is included in languages after r12-6747

[marxin at gcc dot gnu.org]

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

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |marxin at gcc dot gnu.org

--- Comment #2 from Martin Liška <marxin at gcc dot gnu.org> ---
@seurer: Please start using the updated git alises:

        gcc-descr = "!f() { \"`git rev-parse
--show-toplevel`/contrib/git-descr.sh\" $@; } ; f"
        gcc-undescr = "!f() { \"`git rev-parse
--show-toplevel`/contrib/git-undescr.sh\" $@; } ; f"

Thanks.

[Bug target/104335] [12 regression] build failure if go is included in languages after r12-6747

[segher at gcc dot gnu.org]

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

--- Comment #3 from Segher Boessenkool <segher at gcc dot gnu.org> ---
Hi Robin,

Can you please explain what really happens now?  What arguments
rs6000_emit_cmove
is called with, and when that goes wrong?

[Bug target/104335] [12 regression] build failure if go is included in languages after r12-6747

[rdapp at linux dot ibm.com]

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

--- Comment #4 from rdapp at linux dot ibm.com ---
Hi Segher,

originally ifcvt would only pass e.g.

(unle (reg:SF 129 [ _29 ])
    (reg/v:SF 118 [ highScore ]))

as condition to rs6000_emit_cmove via emit_conditional_move ().  (This is the
example from the ICE).

  dest = (reg/f:DI 122 [ bestFuzz$__object ])

The check 

  if (FLOAT_MODE_P (compare_mode) && !FLOAT_MODE_P (result_mode))

fails and we return false i.e. the expander fails and returns NULL_RTX. This is
fine as we will just not do anything with it in ifcvt.

Now with the patch rs6000_emit_cmove is passed

  (unle (reg:CCFP 153)
      (const_int 0 [0])).

This CC comparison has already been computed before so the backend actually
needs to do less work and could just use it without preparing a comparison.
This helps costing on the ifcvt side.

dest is the same as before but now we have

  FLOAT_MODE_P (compare_mode) == false

and

  if (FLOAT_MODE_P (compare_mode) && !FLOAT_MODE_P (result_mode))

not causing a return false.

We then call rs6000_emit_int_cmove which in turn calls rs6000_generate_compare.
There we try to

  emit_insn (gen_rtx_SET (compare_result,                                       
                          gen_rtx_COMPARE (comp_mode, op0, op1)));

but ICE since the modes don't match or make sense.

>From an ifcvt point of view we would just expect a NULL_RTX if something cannot
be handled.  We did not pass CC compares to backends before, so I don't know
how reasonable this assumption is :)

[Bug target/104335] [12 regression] build failure if go is included in languages after r12-6747

[segher at gcc dot gnu.org]

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

--- Comment #5 from Segher Boessenkool <segher at gcc dot gnu.org> ---
(In reply to rdapp from comment #4)
> originally ifcvt would only pass e.g.
> 
> (unle (reg:SF 129 [ _29 ])
>     (reg/v:SF 118 [ highScore ]))
> 
> as condition to rs6000_emit_cmove via emit_conditional_move ().  (This is
> the example from the ICE).

And that works fine, right?

>   dest = (reg/f:DI 122 [ bestFuzz$__object ])
> 
> The check 
> 
>   if (FLOAT_MODE_P (compare_mode) && !FLOAT_MODE_P (result_mode))

With compare_mode SFmode, and result_mode, what, SImode?  Or VOIDmode?
In either case this would be true.

> fails and we return false

Which does not match my argument assumptions then, so they must be wrong.

> i.e. the expander fails and returns NULL_RTX. This
> is fine as we will just not do anything with it in ifcvt.

But it was fine by accident, then :-(

> Now with the patch rs6000_emit_cmove is passed
> 
>   (unle (reg:CCFP 153)
>       (const_int 0 [0])).
>
> This CC comparison has already been computed before so the backend actually
> needs to do less work and could just use it without preparing a comparison.
> This helps costing on the ifcvt side.

But this is done during expand only, costing is pretty irrelevant there.
expand should emit correct code, and later passes optimise that.  Many of our
oldest problems are expand trying to "optimise" things :-(

Or is this function called later as well?

> dest is the same as before

Which is?

> but now we have
> 
>   FLOAT_MODE_P (compare_mode) == false

That is wrong then.

> and
> 
>   if (FLOAT_MODE_P (compare_mode) && !FLOAT_MODE_P (result_mode))
> 
> not causing a return false.

So you meant that FLOAT_MODE (compare_mode) is true?

> We then call rs6000_emit_int_cmove which in turn calls
> rs6000_generate_compare.
> There we try to
> 
>   emit_insn (gen_rtx_SET (compare_result,                                   
> 
>                           gen_rtx_COMPARE (comp_mode, op0, op1)));
> 
> but ICE since the modes don't match or make sense.

Yes, rs6000_generate_compare does not expect the funny RTL for a condition
code as input: it wants a real comparison, instead, so not a representation
of the result of such a comparison.

> From an ifcvt point of view we would just expect a NULL_RTX if something
> cannot be handled.  We did not pass CC compares to backends before, so I
> don't know how reasonable this assumption is :)

Hard to say, I don't find documentation for this.  But, such a change is not
in scope for stage 4, no matter how you look at it :-(

[Bug target/104335] [12 regression] build failure if go is included in languages after r12-6747

[rdapp at linux dot ibm.com]

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

--- Comment #6 from rdapp at linux dot ibm.com ---
Created attachment 52406
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52406&action=edit
Tentative patch

[Bug target/104335] [12 regression] build failure if go is included in languages after r12-6747

[rdapp at linux dot ibm.com]

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

--- Comment #7 from rdapp at linux dot ibm.com ---
Sorry for not getting back to this earlier, talked to Segher off-list about
this quickly.

>> The check 
>>
>>   if (FLOAT_MODE_P (compare_mode) && !FLOAT_MODE_P (result_mode))
> 
> With compare_mode SFmode, and result_mode, what, SImode?  Or VOIDmode?
> In either case this would be true.
> 
>> fails and we return false
> 
> Which does not match my argument assumptions then, so they must be wrong.

We had compare_mode == SFmode and result_mode == DImode before, making the
whole if condition false, causing return false;

> But this is done during expand only, costing is pretty irrelevant there.
> expand should emit correct code, and later passes optimise that.  Many of our
> oldest problems are expand trying to "optimise" things :-(
> 
> Or is this function called later as well?

Yes this does not happen in expand here but during if-convert.

>> dest is the same as before
> 
> Which is?

dest = (reg/f:DI 122 [ bestFuzz$__object ])

>> but now we have
>>
>>   FLOAT_MODE_P (compare_mode) == false
> 
> That is wrong then.
> 
>> and
>>
>>   if (FLOAT_MODE_P (compare_mode) && !FLOAT_MODE_P (result_mode))
>>
>> not causing a return false.
> 
> So you meant that FLOAT_MODE (compare_mode) is true?

The whole condition is now
if (FLOAT_MODE (CCFPmode) && !FLOAT_MODE (DImode))
causing no return false. 

> Yes, rs6000_generate_compare does not expect the funny RTL for a condition
> code as input: it wants a real comparison, instead, so not a representation
> of the result of such a comparison.
> 

Yes you are right that this is not something that is usually expected as a
comparison.  Maybe in the future we can find a better way of informing backends
about both choices.

> Hard to say, I don't find documentation for this.  But, such a change is not
> in scope for stage 4, no matter how you look at it :-(

I agree that I should have at least updated the optab doc for this.  Going to
do so still.

I attached a hack that just returns false whenever a CCmode compare is passed
to  the Power backend functions.  This should get you back the behavior from
before the  patch.  I managed to bootstrap on Power 10 with ./configure
--enable-languages=all --with-cpu=power10 fwiw.  The testsuite ran but I saw
plenty of go FAILS.

Making use of the passed "CC compare"/result is probably a little more involved
but essentially it should be  possible to get rid of all the preparation steps
for CC generation in that case.

[Bug target/104335] [12 regression] build failure if go is included in languages after r12-6747

[segher at gcc dot gnu.org]

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

--- Comment #8 from Segher Boessenkool <segher at gcc dot gnu.org> ---
So I am wondering if this is something we want to do at all.  It seems not
suitable for stage 4 at all.

The problem is that a "comparison" of a CC against 0 is not a comparison
at all, but the result of a previous comparison, instead.  This should be
made much more clear to be allowed in all functions involved if we want to
allow this, and all backends should have time to adjust.  This means it is
only suitable for stage 1.

[Bug target/104335] [12 regression] build failure if go is included in languages after r12-6747

[cvs-commit at gcc dot gnu.org]

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

--- Comment #9 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Robin Dapp <rdapp@gcc.gnu.org>:

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

commit r12-7283-gfac15bf84807a58f83c741b1034c1bc96348319d
Author: Robin Dapp <rdapp@linux.ibm.com>
Date:   Thu Feb 17 19:59:51 2022 +0100

    rs6000: Workaround for new ifcvt behavior [PR104335].

    Since r12-6747-gaa8cfe785953a0 ifcvt passes a "cc comparison"
    i.e. the representation of the result of a comparison to the
    backend.  rs6000_emit_int_cmove () is not prepared to handle this.
    Therefore, this patch makes it return false in such a case.

            PR target/104335

    gcc/ChangeLog:

            * config/rs6000/rs6000.cc (rs6000_emit_int_cmove): Return false
            if the expected comparison's first operand is of mode MODE_CC.

[Bug target/104335] [12 regression] build failure if go is included in languages after r12-6747

[meissner at gcc dot gnu.org]

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

Michael Meissner <meissner at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |asolokha at gmx dot com

--- Comment #10 from Michael Meissner <meissner at gcc dot gnu.org> ---
*** Bug 104256 has been marked as a duplicate of this bug. ***

[Bug target/104335] [12 regression] build failure if go is included in languages after r12-6747

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Is this fixed?

[Bug target/104335] [12 regression] build failure if go is included in languages after r12-6747

[seurer at gcc dot gnu.org]

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

seurer at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #12 from seurer at gcc dot gnu.org ---
This no longer fails.