[Bug middle-end/106075] New: Wrong DSE with -fnon-call-exceptions

[Bug middle-end/106075] New: Wrong DSE with -fnon-call-exceptions

[hubicka at gcc dot gnu.org]

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

            Bug ID: 106075
           Summary: Wrong DSE with -fnon-call-exceptions
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: middle-end
          Assignee: unassigned at gcc dot gnu.org
          Reporter: hubicka at gcc dot gnu.org
  Target Milestone: ---

In the following testcase:

int a = 1;
short *b;
void
test()
{
        a=12345;
        *b=0;
        a=1;
}

we should not optimize out a=12345 when compiling with -fnon-call-exceptions. 
The store to b may throw and thus the store is live.

[Bug middle-end/106075] Wrong DSE with -fnon-call-exceptions

[pinskia at gcc dot gnu.org]

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

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
There is another option to not to remove stores for non call exceptions.

[Bug middle-end/106075] Wrong DSE with -fnon-call-exceptions

[pinskia at gcc dot gnu.org]

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

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Oh wait -fdelete-dead-exceptions won't change that here or will ir.

[Bug middle-end/106075] Wrong DSE with -fnon-call-exceptions

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2022-06-24
                 CC|                            |rguenth at gcc dot gnu.org

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
Related:

int a = 1;
int c,d;
void
test()
{
  a=12345;
  c/d;
  a=1;
}

where the possibly throwing division (by zero) does not have virtual operands.
Likewise

void __attribute__((noreturn,const)) foo ()
{
  longjmp (&buf);
}

int a = 1;
int c,d;
void
test()
{
  a=12345;
  foo ();
  a=1;
}

but then we can simply declare 'const' invalid on 'foo'.

For the non-VOP case we'd need to assign a "context" counter to stmts
(in UID for example) and increment it when seeing a possible (implicit)
control flow terminating statement.  When the DSE walk discovers a new
context it has to consider that a use.  The expense is an extra whole-IL
walk over the function [with -fnon-call-exceptions].  Note there's also
the possibility to create a const externally throwing function but its
semantics are disputed (see another PR for that).

[Bug middle-end/106075] Wrong DSE with -fnon-call-exceptions

[hubicka at gcc dot gnu.org]

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

--- Comment #4 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
PR106077 demonstrates related problem where ipa-sra concludes it is safe to
move dereference earlier in the code.  It uses dominator test for that.

[Bug middle-end/106075] Wrong DSE with -fnon-call-exceptions

[hubicka at gcc dot gnu.org]

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

--- Comment #5 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
Also note that the longjmp testcase will not get misoptimized since we consider
longjmp as using all global memory.

[Bug middle-end/106075] Wrong DSE with -fnon-call-exceptions

[hubicka at gcc dot gnu.org]

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

--- Comment #6 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
The SRA issue is fixed now, but I am not quite sure what is desrable solution
here...

This blocks modref from understanding side effects of functions doing EH.

[Bug middle-end/106075] Wrong DSE with -fnon-call-exceptions

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |rguenth at gcc dot gnu.org

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
I think the IL is somewhat buggy in the control transfer instruction not being
indicated as reading from global memory.  I think it should act similar to
a GIMPLE_RETURN where we now force a VUSE.  There's a related bug about
GIMPLE_RESX not having virtual operands and the issue DSE has with this.

Now, that would mean that for the original testcase a fix would be simply
to consider all throwing points uses (for global memory).  I have a patch
to do that, leaving the IL issue (the throwing division case and RESX)
unfixed.  Note that RTL DSE happily removes the first store as well, so
I had to fix that as well.

[Bug middle-end/106075] Wrong DSE with -fnon-call-exceptions

[rguenth at gcc dot gnu.org]

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

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
Created attachment 54979
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54979&action=edit
patch

This patch fixes the testcase but it causes Ada and Go failures.  I've
isolated the Ada FAILs to the dse.cc change but then I'm not really into
this RTL optimization pass nor the pecularities of Ada (or Go).

                === acats tests ===
FAIL:   c37009a
FAIL:   c37208a
FAIL:   c41103a
FAIL:   c41103b
FAIL:   c41301a
FAIL:   c55c02b
FAIL:   c87b40a
FAIL:   cc1018a 

                === gnat tests ===


Running target unix/
FAIL: gnat.dg/opt47.adb execution test

                === go tests ===


Running target unix/
FAIL: go.go-torture/execute/names-1.go execution,  -O1
FAIL: go.go-torture/execute/names-1.go execution,  -O2
FAIL: go.go-torture/execute/names-1.go execution,  -O2 -fbounds-check
FAIL: go.go-torture/execute/names-1.go execution,  -O2 -fomit-frame-pointer
-finline-functions
FAIL: go.go-torture/execute/names-1.go execution,  -O2 -fomit-frame-pointer
-finline-functions -funroll-loops
FAIL: go.go-torture/execute/names-1.go execution,  -O3 -g
FAIL: ./index0-out.go execution,  -O0 -g -fno-var-tracking-assignments
FAIL: go.test/test/235.go execution,  -O2 -g
FAIL: go.test/test/chan/sieve1.go execution,  -O2 -g
FAIL: go.test/test/chan/sieve2.go execution,  -O2 -g
FAIL: go.test/test/cmp.go execution,  -O2 -g
FAIL: go.test/test/complit.go execution,  -O2 -g
FAIL: go.test/test/ddd.go execution,  -O2 -g
FAIL: go.test/test/fixedbugs/bug187.go execution,  -O2 -g
FAIL: go.test/test/fixedbugs/bug204.go execution,  -O2 -g
FAIL: go.test/test/fixedbugs/bug447.go execution
FAIL: go.test/test/fixedbugs/bug465.dir/b.go execution,  -O2 -g
FAIL: go.test/test/fixedbugs/bug483.go execution,  -O2 -g
FAIL: go.test/test/fixedbugs/issue10925.go execution,  -O2 -g
FAIL: go.test/test/fixedbugs/issue14553.go execution,  -O2 -g
FAIL: go.test/test/fixedbugs/issue15528.go execution,  -O2 -g
FAIL: go.test/test/fixedbugs/issue16095.go execution,  -O2 -g
FAIL: go.test/test/fixedbugs/issue19113.go execution,  -O2 -g
FAIL: go.test/test/fixedbugs/issue22881.go execution,  -O2 -g
FAIL: go.test/test/fixedbugs/issue27836.dir/Ã~Dfoo.go  -O -I. (test for excess
errors)
FAIL: go.test/test/fixedbugs/issue27836.dir/Ã~Dmain.go  -O -I. (test for excess
errors)
FAIL: go.test/test/fixedbugs/issue39505b.go execution,  -O2 -g
FAIL: go.test/test/fixedbugs/issue4909b.go execution
FAIL: go.test/test/fixedbugs/issue5162.go execution
FAIL: go.test/test/fixedbugs/issue9604b.go execution
FAIL: go.test/test/ken/range.go execution,  -O2 -g
FAIL: go.test/test/slice3.go execution
FAIL: go.test/test/stringrange.go execution,  -O2 -g 
FAIL: go.test/test/utf.go execution,  -O2 -g 

(also gotools and libgo fails)

[Bug middle-end/106075] Wrong DSE with -fnon-call-exceptions

[rguenth at gcc dot gnu.org]

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

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

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

--- Comment #9 from Richard Biener <rguenth at gcc dot gnu.org> ---
Eric, maybe you can help with RTL DSE here?

[Bug middle-end/106075] Wrong DSE with -fnon-call-exceptions

[ebotcazou at gcc dot gnu.org]

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

--- Comment #10 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> Eric, maybe you can help with RTL DSE here?

Sure, let me have a look.

[Bug middle-end/106075] Wrong DSE with -fnon-call-exceptions

[ebotcazou at gcc dot gnu.org]

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

--- Comment #11 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> Eric, maybe you can help with RTL DSE here?

The problem is that add_non_frame_wild_read is exclusive, i.e. the read is
turned into a read from *only* non-frame memory, but can_throw_external
is conservative and may return true for a read from frame memory:

(insn 15 13 16 4 (set (reg:QI 87 [ _20 ])
        (mem:QI (plus:DI (reg/f:DI 100)
                (reg:DI 92 [ ivtmp.23 ])) [8 MEM <character>
[(interfaces__unsigned_32 *)&value.4 + ivtmp.23_39 * 1]+0 S1 A8]))
"opt47.adb":22:13 91 {*movqi_internal}

value.4 is a local variable but can_throw_external return true on the read.

So you would need to implement an inclusive version of add_non_frame_wild_read
but the change is going to pessimize Ada and Go if the condition to invoke it
is not made more precise.

[Bug middle-end/106075] Wrong DSE with -fnon-call-exceptions

[rguenther at suse dot de]

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

--- Comment #12 from rguenther at suse dot de <rguenther at suse dot de> ---
On Mon, 8 May 2023, ebotcazou at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106075
> 
> --- Comment #11 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> > Eric, maybe you can help with RTL DSE here?
> 
> The problem is that add_non_frame_wild_read is exclusive, i.e. the read is
> turned into a read from *only* non-frame memory, but can_throw_external
> is conservative and may return true for a read from frame memory:
> 
> (insn 15 13 16 4 (set (reg:QI 87 [ _20 ])
>         (mem:QI (plus:DI (reg/f:DI 100)
>                 (reg:DI 92 [ ivtmp.23 ])) [8 MEM <character>
> [(interfaces__unsigned_32 *)&value.4 + ivtmp.23_39 * 1]+0 S1 A8]))
> "opt47.adb":22:13 91 {*movqi_internal}
> 
> value.4 is a local variable but can_throw_external return true on the read.

Ah - interesting.  I wonder how the CALL handling is correct in
using add_non_frame_wild_read then for the case we pass the address
of a frame variable to it.

So I suppose add_wild_read () would do the trick.

> So you would need to implement an inclusive version of add_non_frame_wild_read
> but the change is going to pessimize Ada and Go if the condition to invoke it
> is not made more precise.

So better would be to include the original effects of the insn _plus_
the non-frame wild read effect.

I'm defering this for now, the problem went unnoticed for long, so ...

[Bug middle-end/106075] Wrong DSE with -fnon-call-exceptions

[rguenth at gcc dot gnu.org]

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

--- Comment #13 from Richard Biener <rguenth at gcc dot gnu.org> ---
Maybe just setting insn_info->non_frame_wild_read to true will work? 
add_non_frame_wild_read is odd in that it removes _all_ read records,
not only non-frame related ones, same as add_wild_read (but we don't
have whether we deal with frame or non-frame mem recorded in read_info_type).

[Bug middle-end/106075] Wrong DSE with -fnon-call-exceptions

[ebotcazou at gcc dot gnu.org]

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

--- Comment #14 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> Ah - interesting.  I wonder how the CALL handling is correct in
> using add_non_frame_wild_read then for the case we pass the address
> of a frame variable to it.

See kill_on_calls.

> So I suppose add_wild_read () would do the trick.

Nope, since it also calls free_read_records.

> So better would be to include the original effects of the insn _plus_
> the non-frame wild read effect.

Yes, exactly.

[Bug middle-end/106075] Wrong DSE with -fnon-call-exceptions

[ebotcazou at gcc dot gnu.org]

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

--- Comment #15 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> Maybe just setting insn_info->non_frame_wild_read to true will work? 

Possibly.