[Bug tree-optimization/101373] New: PRE hoists trapping instructions over possibly throwing calls

[Bug tree-optimization/101373] New: PRE hoists trapping instructions over possibly throwing calls

[rguenth at gcc dot gnu.org]

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

            Bug ID: 101373
           Summary: PRE hoists trapping instructions over possibly
                    throwing calls
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: rguenth at gcc dot gnu.org
  Target Milestone: ---

int __attribute__((const,noipa)) foo (int j)
{
  if (j != 0)
    throw 1;
  return 0;
}

int __attribute__((noipa)) bar (int *p, int n)
{
  int ret = 0;
  if (n)
    {
       foo (n);
       ret = *p;
    }
  ret += *p;
  return ret;
}

int main()
{
  try
    {
      return bar (nullptr, 1);
    }
  catch (...)
    {
      return 0;
    }
}


Here PRE hoists *p before the if (n) which it may not do.

[Bug tree-optimization/101373] PRE hoists trapping instructions over possibly throwing calls

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Depends on|                            |100409
   Last reconfirmed|                            |2021-07-08
             Status|UNCONFIRMED                 |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |rguenth at gcc dot gnu.org
           Keywords|                            |wrong-code
     Ever confirmed|0                           |1

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
Testing a patch.  Depends on a fix for PR100409 which else breaks the testcase
in the C++ frontend already.


Referenced Bugs:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100409
[Bug 100409] C++ FE elides pure throwing call

[Bug tree-optimization/101373] PRE hoists trapping instructions over possibly throwing calls

[rguenth at gcc dot gnu.org]

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

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

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

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
Eric, is it possible to write an equivalent testcase in Ada?  I don't seem to
get any progress with the C++ FE miscompiling this from the start (PR100409)
but
I do have a patch for this issue (but I don't want to fix it without adding a
testcase).

[Bug tree-optimization/101373] PRE hoists trapping instructions over possibly throwing calls

[ebotcazou at gcc dot gnu.org]

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

--- Comment #3 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> Eric, is it possible to write an equivalent testcase in Ada?

procedure P is

  function Foo (J : Integer) return Integer;
  pragma Machine_Attribute (Foo, "const");
  pragma Machine_Attribute (Foo, "noipa");

  function Foo (J : Integer) return Integer is
  begin
    if J /= 0 then
      raise Constraint_Error;
    end if;
    return 0;
  end;

  function Bar (A : access Integer; N : Integer) return Integer;
  pragma Machine_Attribute (Bar, "noipa");

  function Bar (A : access Integer; N : Integer) return Integer is
    Ret : Integer := 0;
  begin
    if N /= 0 then
      Ret := Foo (N);
      Ret := A.all;
    end if;
    Ret := Ret + A.all;
    return Ret;
  end;

  V : Integer;
  pragma Volatile (V);

begin
  V := Bar (null, 1);
exception
  when Constraint_Error => null;
end;

compiled with -O2 -gnatp, but I cannot have the load hoisted in this case.

[Bug tree-optimization/101373] PRE hoists trapping instructions over possibly throwing calls

[rguenther at suse dot de]

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

--- Comment #4 from rguenther at suse dot de <rguenther at suse dot de> ---
On Fri, 9 Jul 2021, ebotcazou at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101373
> 
> --- Comment #3 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> > Eric, is it possible to write an equivalent testcase in Ada?
> 
> procedure P is
> 
>   function Foo (J : Integer) return Integer;
>   pragma Machine_Attribute (Foo, "const");
>   pragma Machine_Attribute (Foo, "noipa");
> 
>   function Foo (J : Integer) return Integer is
>   begin
>     if J /= 0 then
>       raise Constraint_Error;
>     end if;
>     return 0;
>   end;
> 
>   function Bar (A : access Integer; N : Integer) return Integer;
>   pragma Machine_Attribute (Bar, "noipa");
> 
>   function Bar (A : access Integer; N : Integer) return Integer is
>     Ret : Integer := 0;
>   begin
>     if N /= 0 then
>       Ret := Foo (N);
>       Ret := A.all;
>     end if;
>     Ret := Ret + A.all;
>     return Ret;
>   end;
> 
>   V : Integer;
>   pragma Volatile (V);
> 
> begin
>   V := Bar (null, 1);
> exception
>   when Constraint_Error => null;
> end;
> 
> compiled with -O2 -gnatp, but I cannot have the load hoisted in this case.

The issue I can see is that the middle-end does not consider
the Foo function 'const' (gimple_call_flags does not return ECF_CONST)
which is likely because the decl isn't TREE_READONLY and for 'const'
we nowhere lookup the actual attribute but handlers are expected to
mimic c-family/c-attribs.c handling.

With

diff --git a/gcc/gimple.c b/gcc/gimple.c
index 60a90667e4b..576b5df5cec 100644
--- a/gcc/gimple.c
+++ b/gcc/gimple.c
@@ -1470,7 +1470,11 @@ gimple_call_flags (const gimple *stmt)
     {
       tree decl = gimple_call_fndecl (stmt);
       if (decl)
-       flags = flags_from_decl_or_type (decl);
+       {
+         flags = flags_from_decl_or_type (decl);
+         if (lookup_attribute ("const", DECL_ATTRIBUTES (decl)))
+           flags |= ECF_CONST;
+       }
       flags |= flags_from_decl_or_type (gimple_call_fntype (stmt));
     }

I see the access hoisted (but as said, the Ada FE should set
TREE_READONLY here).

I first thought that non-call exceptions might be the issue
(we wouldn't move A.all then), but non-call exceptions seem off.

[Bug tree-optimization/101373] PRE hoists trapping instructions over possibly throwing calls

[ebotcazou at gcc dot gnu.org]

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

--- Comment #5 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> The issue I can see is that the middle-end does not consider
> the Foo function 'const' (gimple_call_flags does not return ECF_CONST)
> which is likely because the decl isn't TREE_READONLY and for 'const'
> we nowhere lookup the actual attribute but handlers are expected to
> mimic c-family/c-attribs.c handling.

We do that, but then the flag is overwritten...  The fix is just:

diff --git a/gcc/ada/gcc-interface/utils.c b/gcc/ada/gcc-interface/utils.c
index 952f032072b..bfacb0eaeb4 100644
--- a/gcc/ada/gcc-interface/utils.c
+++ b/gcc/ada/gcc-interface/utils.c
@@ -3543,9 +3543,6 @@ finish_subprog_decl (tree decl, tree asm_name, tree type)
   DECL_BY_REFERENCE (result_decl) = TREE_ADDRESSABLE (type);
   DECL_RESULT (decl) = result_decl;

-  /* Propagate the "const" property.  */
-  TREE_READONLY (decl) = TYPE_READONLY (type);
-
   /* Propagate the "pure" property.  */
   DECL_PURE_P (decl) = TYPE_RESTRICT (type);

> I first thought that non-call exceptions might be the issue
> (we wouldn't move A.all then), but non-call exceptions seem off.

Yes, -gnatp disables -fnon-call-exceptions.

[Bug tree-optimization/101373] PRE hoists trapping instructions over possibly throwing calls

[rguenth at gcc dot gnu.org]

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

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

This is the patch fixing the issue for the C++ testcase (when a fix for
PR100409 is applied), it also fixes the Ada testcase to not perform the
hoisting with
my hack active and probably with your real fix.

I've not yet managed to verify that the gnat.dg/opt95.adb testcase works
as expected but will do so when the Ada FE is officially fixed.

The patch also touches RTL (postreload) PRE as both suffer from the same
issue as GIMPLE PRE.  At least for the C++ testcase it shows the RTL PRE
issue at -Os.

[Bug tree-optimization/101373] PRE hoists trapping instructions over possibly throwing calls

[ebotcazou at gcc dot gnu.org]

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

--- Comment #7 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> I've not yet managed to verify that the gnat.dg/opt95.adb testcase works
> as expected but will do so when the Ada FE is officially fixed.

Done.

[Bug tree-optimization/101373] PRE hoists trapping instructions over possibly throwing calls

[ebotcazou at gcc dot gnu.org]

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

--- Comment #8 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
Note that the testcase also raises Storage_Error with:

  function Foo (J : Integer) return Integer;
  pragma Pure_Function (Foo);
  pragma Machine_Attribute (Foo, "noipa");

and in Ada calls to pure functions are allowed to be elided if their result is
not used, independently of whether they may raise an exception, so we
effectively end up with something like:

  function Bar (A : access Integer; N : Integer) return Integer is
    Ret : Integer := 0;
  begin
    if N /= 0 then
      Ret := A.all;
    end if;
    Ret := Ret + A.all;
    return Ret;
  end;

for which raising Storage_Error is sort of expected with -gnatp.

[Bug tree-optimization/101373] PRE hoists trapping instructions over possibly throwing calls

[ebotcazou at gcc dot gnu.org]

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

--- Comment #9 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
Or maybe I misunderstood your request and you wanted an Ada testcase where the
hoisting would still happen despite the fix for C++?

[Bug tree-optimization/101373] PRE hoists trapping instructions over possibly throwing calls

[rguenther at suse dot de]

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

--- Comment #10 from rguenther at suse dot de <rguenther at suse dot de> ---
On July 9, 2021 9:43:52 PM GMT+02:00, "ebotcazou at gcc dot gnu.org"
<gcc-bugzilla@gcc.gnu.org> wrote:
>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101373
>
>--- Comment #9 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
>Or maybe I misunderstood your request and you wanted an Ada testcase
>where the
>hoisting would still happen despite the fix for C++?

No, I thought we could have one that's fixed when the hoisting does not
happen...

[Bug tree-optimization/101373] PRE hoists trapping instructions over possibly throwing calls

[rguenther at suse dot de]

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

--- Comment #11 from rguenther at suse dot de <rguenther at suse dot de> ---
On July 10, 2021 7:13:47 PM GMT+02:00, rguenther at suse dot de
<gcc-bugzilla@gcc.gnu.org> wrote:
>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101373
>
>--- Comment #10 from rguenther at suse dot de <rguenther at suse dot
>de> ---
>On July 9, 2021 9:43:52 PM GMT+02:00, "ebotcazou at gcc dot gnu.org"
><gcc-bugzilla@gcc.gnu.org> wrote:
>>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101373
>>
>>--- Comment #9 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
>>Or maybe I misunderstood your request and you wanted an Ada testcase
>>where the
>>hoisting would still happen despite the fix for C++?
>
>No, I thought we could have one that's fixed when the hoisting does not
>happen...

Maybe for that we just have to use the result of the call in the non-throwing
case.

[Bug tree-optimization/101373] PRE hoists trapping instructions over possibly throwing calls

[ebotcazou at gcc dot gnu.org]

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

--- Comment #12 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> Maybe for that we just have to use the result of the call in the
> non-throwing case.

That seems to block the hoisting in all cases.

[Bug tree-optimization/101373] PRE hoists trapping instructions over possibly throwing calls

[rguenth at gcc dot gnu.org]

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

--- Comment #13 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Eric Botcazou from comment #12)
> > Maybe for that we just have to use the result of the call in the
> > non-throwing case.
> 
> That seems to block the hoisting in all cases.

  function Bar (A : access Integer; N : Integer) return Integer is
    Ret : Integer := 0;
    Ret2 : Integer := 0;
  begin
    if N /= 0 then
      Ret2 := Foo (N);
      Ret := A.all;
    end if;
    Ret := Ret + A.all;
    return Ret + Ret2;
  end;

seems to work as in, FAIL without the patch and PASS with it.

[Bug tree-optimization/101373] PRE hoists trapping instructions over possibly throwing calls

[ebotcazou at gcc dot gnu.org]

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

--- Comment #14 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
>   function Bar (A : access Integer; N : Integer) return Integer is
>     Ret : Integer := 0;
>     Ret2 : Integer := 0;
>   begin
>     if N /= 0 then
>       Ret2 := Foo (N);
>       Ret := A.all;
>     end if;
>     Ret := Ret + A.all;
>     return Ret + Ret2;
>   end;
> 
> seems to work as in, FAIL without the patch and PASS with it.

OK, I can reproduce it.  Please use the Pure_Function version for gnat.dg:

  function Foo (J : Integer) return Integer;
  pragma Pure_Function (Foo);
  pragma Machine_Attribute (Foo, "noipa");

[Bug tree-optimization/101373] PRE hoists trapping instructions over possibly throwing calls

[cvs-commit at gcc dot gnu.org]

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

--- Comment #15 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

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

commit r12-2254-gfedcf3c476aff7533741a1c61071200f0a38cf83
Author: Richard Biener <rguenther@suse.de>
Date:   Thu Jul 8 09:52:49 2021 +0200

    tree-optimization/101373 - avoid PRE across externally throwing call

    PRE already tries to avoid hoisting possibly trapping expressions
    across calls that might not return normally but fails to consider
    const calls that throw externally.  The following fixes that and
    also plugs the hole of trapping references not pruned in case
    they are not catched by the actuall call clobbering it.

    At -Os we hit the same issue in RTL PRE and postreload-gcse has
    even more incomplete checks so the patch adjusts both of those
    as well.

    2021-07-08  Richard Biener  <rguenther@suse.de>

            PR tree-optimization/101373
            * tree-ssa-pre.c (prune_clobbered_mems): Also prune trapping
            references when the BB may not return.
            (compute_avail): Pass in the function we're working on and
            replace cfun references with it.  Externally throwing
            const calls also possibly terminate the function.
            (pass_pre::execute): Pass down the function we're working on.
            * gcse.c (compute_hash_table_work): Externally throwing
            const/pure calls also need record_last_mem_set_info.
            * postreload-gcse.c (record_opr_changes): Looping or externally
            throwing const/pure calls also need record_last_mem_set_info.

            * g++.dg/torture/pr101373.C: New testcase, XFAILed.
            * gnat.dg/opt95.adb: Likewise.

[Bug tree-optimization/101373] PRE hoists trapping instructions over possibly throwing calls

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to work|                            |12.0
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #16 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed on trunk.

[Bug tree-optimization/101373] PRE hoists trapping instructions over possibly throwing calls

[cvs-commit at gcc dot gnu.org]

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

--- Comment #17 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Richard Biener
<rguenth@gcc.gnu.org>:

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

commit r11-8875-gee875b63b22e30a0dcb4b05f7532c2c416ba6cd0
Author: Richard Biener <rguenther@suse.de>
Date:   Tue Aug 17 08:38:35 2021 +0200

    tree-optimization/101868 - avoid PRE of trapping mems across calls

    This backports a fix for the omission of a check of trapping mems
    when hoisting them across calls that might not return.  This was
    originally done as part of a fix to handle const functions that throw
    properly.

    2021-08-17  Richard Biener  <rguenther@suse.de>

            PR tree-optimization/101373
            PR tree-optimization/101868
            * tree-ssa-pre.c (prune_clobbered_mems): Also prune trapping
            references when the BB may not return.

            * gcc.dg/lto/pr101868_0.c: New testcase.
            * gcc.dg/lto/pr101868_1.c: Likewise.
            * gcc.dg/lto/pr101868_2.c: Likewise.
            * gcc.dg/lto/pr101868_3.c: Likewise.

[Bug tree-optimization/101373] PRE hoists trapping instructions over possibly throwing calls

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |11.3

[Bug tree-optimization/101373] PRE hoists trapping instructions over possibly throwing calls

[cvs-commit at gcc dot gnu.org]

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

--- Comment #18 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Richard Biener
<rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:95a95ec274cd0ec125ce48ab002fad4e400e345b

commit r10-10206-g95a95ec274cd0ec125ce48ab002fad4e400e345b
Author: Richard Biener <rguenther@suse.de>
Date:   Tue Aug 17 08:38:35 2021 +0200

    tree-optimization/101868 - avoid PRE of trapping mems across calls

    This backports a fix for the omission of a check of trapping mems
    when hoisting them across calls that might not return.  This was
    originally done as part of a fix to handle const functions that throw
    properly.

    2021-08-17  Richard Biener  <rguenther@suse.de>

            PR tree-optimization/101373
            PR tree-optimization/101868
            * tree-ssa-pre.c (prune_clobbered_mems): Also prune trapping
            references when the BB may not return.

            * gcc.dg/lto/pr101868_0.c: New testcase.
            * gcc.dg/lto/pr101868_1.c: Likewise.
            * gcc.dg/lto/pr101868_2.c: Likewise.
            * gcc.dg/lto/pr101868_3.c: Likewise.

    (cherry picked from commit ee875b63b22e30a0dcb4b05f7532c2c416ba6cd0)

[Bug tree-optimization/101373] PRE hoists trapping instructions over possibly throwing calls

[cvs-commit at gcc dot gnu.org]

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

--- Comment #19 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-9 branch has been updated by Richard Biener
<rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:2498de689b735422ef71d93e2afe7ae3e6988bb3

commit r9-9818-g2498de689b735422ef71d93e2afe7ae3e6988bb3
Author: Richard Biener <rguenther@suse.de>
Date:   Tue Aug 17 08:38:35 2021 +0200

    tree-optimization/101868 - avoid PRE of trapping mems across calls

    This backports a fix for the omission of a check of trapping mems
    when hoisting them across calls that might not return.  This was
    originally done as part of a fix to handle const functions that throw
    properly.

    2021-08-17  Richard Biener  <rguenther@suse.de>

            PR tree-optimization/101373
            PR tree-optimization/101868
            * tree-ssa-pre.c (prune_clobbered_mems): Also prune trapping
            references when the BB may not return.

            * gcc.dg/lto/pr101868_0.c: New testcase.
            * gcc.dg/lto/pr101868_1.c: Likewise.
            * gcc.dg/lto/pr101868_2.c: Likewise.
            * gcc.dg/lto/pr101868_3.c: Likewise.

    (cherry picked from commit ee875b63b22e30a0dcb4b05f7532c2c416ba6cd0)

[Bug tree-optimization/101373] PRE hoists trapping instructions over possibly throwing calls

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101373
Bug 101373 depends on bug 100409, which changed state.

Bug 100409 Summary: C++ FE elides pure throwing call
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100409

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