This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Remove also 2nd argument for unused delete operator (PR tree-optimization/91270).


On Mon, Jul 29, 2019 at 12:37 PM Martin Liška <mliska@suse.cz> wrote:
>
> On 7/29/19 11:59 AM, Richard Biener wrote:
> > On Sun, Jul 28, 2019 at 4:57 PM Martin Liška <mliska@suse.cz> wrote:
> >>
> >> Hi.
> >>
> >> And there's one more patch that deals with delete operator
> >> which has a second argument (size). That definition GIMPLE
> >> statement of the size must be also properly marked.
> >>
> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> >>
> >> Ready to be installed?
> >
> > This isn't a proper fix.  You can't mark a random operand as necessary
> > during elimination - this only works in very constraint cases.  Here
> > it will break down if the stmt you just marked depends on another stmt
> > only used by the stmt you just marked (and thus also not necessary).
>
> Ah, I see.
>
> >
> > The fix is to propagate_necessity where you either have to excempt
> > the two-operator delete from handling
>
> We want to process also these delete operators.
>
> > or to mark its second operand
> > as necessary despite eventually deleting the call.
>
> Really marking that as necessary? Why?
>
> > You can avoid
> > this in case the allocation function used the exact same size argument.
>
> That's not the case as the operator new happens in a loop:
>
>   <bb 5> :
>   # iftmp.0_15 = PHI <iftmp.0_21(3), iftmp.0_20(4)>
>   _23 = operator new [] (iftmp.0_15);
>   _24 = _23;
>   MEM[(sizetype *)_24] = _19;
>   _26 = _24 + 8;
>   _27 = _26;
>   _2 = _19 + 18446744073709551615;
>   _28 = (long int) _2;
>
>   <bb 6> :
>   # _12 = PHI <_27(5), _29(7)>
>   # _13 = PHI <_28(5), _30(7)>
>   if (_13 < 0)
>     goto <bb 8>; [INV]
>   else
>     goto <bb 7>; [INV]
>
> Btw. is the hunk moved to propagate_necessity still wrong:
>
> diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c
> index cf507fa0453..1c890889932 100644
> --- a/gcc/tree-ssa-dce.c
> +++ b/gcc/tree-ssa-dce.c
> @@ -803,7 +803,23 @@ propagate_necessity (bool aggressive)
>                            || DECL_FUNCTION_CODE (def_callee) == BUILT_IN_MALLOC
>                            || DECL_FUNCTION_CODE (def_callee) == BUILT_IN_CALLOC))
>                       || DECL_IS_REPLACEABLE_OPERATOR_NEW_P (def_callee)))
> -               continue;
> +               {
> +                 /* Some delete operators have 2 arguments, where
> +                    the second argument is size of the deallocated memory.  */
> +                 if (gimple_call_num_args (stmt) == 2)

Please make sure this only matches operator delete (just make the check
we already do above also set a bool flag).

> +                   {
> +                     tree ptr = gimple_call_arg (stmt, 1);
> +                     if (TREE_CODE (ptr) == SSA_NAME)

you can use mark_operand_necessary (ptr) here (but please name 'ptr'
differently ;))

And note you can elide this in case the size arguments to new and delete
match.  I notice the above also matches

   ptr = malloc (4);
   delete ptr;

not sure if intended (or harmful).

Richard.

> +                       {
> +                         gimple *def_stmt = SSA_NAME_DEF_STMT (ptr);
> +                         if (!gimple_nop_p (def_stmt)
> +                             && !gimple_plf (def_stmt, STMT_NECESSARY))
> +                           gimple_set_plf (stmt, STMT_NECESSARY, false);
> +                       }
> +                   }
> +
> +                 continue;
> +               }
>             }
>
>           FOR_EACH_SSA_TREE_OPERAND (use, stmt, iter, SSA_OP_USE)
>
> Thanks,
> Martin
>
> >
> > Richard.
> >
> >> Thanks,
> >> Martin
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]