Hi, On Tue, Dec 13 2022, Richard Biener wrote: > On Mon, 12 Dec 2022, Jan Hubicka wrote: > >> > > Hi, >> > > >> > > I'm re-posting patches which I have posted at the end of stage 1 but >> > > which have not passed review yet. >> > > >> > > 8<-------------------------------------------------------------------- >> > > >> > > I have noticed that scan_expr_access passes all the expressions it >> > > gets to get_ref_base_and_extent even when we are really only >> > > interested in memory accesses. So bail out when the expression is >> > > something clearly uninteresting. >> > > >> > > Bootstrapped and tested individually when I originally posted it and >> > > now bootstrapped and LTO-bootstrapped and tested as part of the whole >> > > series. OK for master? >> > > >> > > >> > > gcc/ChangeLog: >> > > >> > > 2021-12-14 Martin Jambor <mjam...@suse.cz> >> > > >> > > * ipa-sra.c (scan_expr_access): Bail out early if expr is something we >> > > clearly do not need to pass to get_ref_base_and_extent. >> > > --- >> > > gcc/ipa-sra.cc | 5 +++++ >> > > 1 file changed, 5 insertions(+) >> > > >> > > diff --git a/gcc/ipa-sra.cc b/gcc/ipa-sra.cc >> > > index 93fceeafc73..3646d71468c 100644 >> > > --- a/gcc/ipa-sra.cc >> > > +++ b/gcc/ipa-sra.cc >> > > @@ -1748,6 +1748,11 @@ scan_expr_access (tree expr, gimple *stmt, >> > > isra_scan_context ctx, >> > > || TREE_CODE (expr) == REALPART_EXPR) >> > > expr = TREE_OPERAND (expr, 0); >> > > >> > > + if (!handled_component_p (expr) >> > > + && !DECL_P (expr) >> > > + && TREE_CODE (expr) != MEM_REF) >> > > + return; >> > Is this needed because get_ref_base_and_extend crashes if given SSA_NAME >> > or something else or is it just optimization? >> > Perhaps Richi will know if there is better test for this. > > Also the code preceeding the above > > if (TREE_CODE (expr) == BIT_FIELD_REF > || TREE_CODE (expr) == IMAGPART_EXPR > || TREE_CODE (expr) == REALPART_EXPR) > expr = TREE_OPERAND (expr, 0); > > but get_ref_base_and_extent shouldn't crash on anything here. The > question is what you want 'expr' to be? The comment of the function > says CTX specifies that, but doesn't constrain the CALL case (does > it have to be a memory argument)? > > With allowing handled_component_p but above not handling > VIEW_CONVERT_EXPR you leave the possibility of VIEW_CONVERT_EXPR (d_1) > slipping through. Since the non-memory cases will have at most > one wrapping handled_component get_ref_base_and_extent should be > reasonably cheap, so maybe just cut off SSA_NAME, ADDR_EXPR and > CONSTANT_CLASS_P at the start of the function? >
The patch was intended just as a simple optimization in order not to run get_ref_base_and_extent on stuff where one can see from the top-most tree they the result won't be interesting. Indeed it looks like get_ref_base_and_extent does not really need this when run on non-loads. I'll think about the function a bit more but it seems like the patch just is not really necessary. Thanks, Martin
