* [PATCH] go/100537 - Bootstrap-O3 and bootstrap-debug fail @ 2021-05-14 2:52 Jiufu Guo 2021-05-14 7:15 ` Richard Biener 0 siblings, 1 reply; 11+ messages in thread From: Jiufu Guo @ 2021-05-14 2:52 UTC (permalink / raw) To: gcc-patches; +Cc: guojiufu, wschmidt, rguenther, ian As discussed in the PR, Richard mentioned the method to figure out which VAR was not set TREE_ADDRESSABLE, and then cause this failure. It is address_expression which build addr_expr (build_fold_addr_expr_loc), but not set TREE_ADDRESSABLE. I drafted this patch with reference the comments from Richard in this PR, while I'm not quite sure if more thing need to do. So, please have review, thanks! Bootstrap and regtest pass on ppc64le. Is this ok for trunk? Jiufu Guo. 2021-05-14 Richard Biener <rguenther@suse.de> Jiufu Guo <guojiufu@linux.ibm.com> PR go/100537 * go-gcc.cc (Gcc_backend::address_expression): Set TREE_ADDRESSABLE. --- gcc/go/go-gcc.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc index 5d9dbb5d068..8ed20a3b479 100644 --- a/gcc/go/go-gcc.cc +++ b/gcc/go/go-gcc.cc @@ -1680,6 +1680,7 @@ Gcc_backend::address_expression(Bexpression* bexpr, Location location) if (expr == error_mark_node) return this->error_expression(); + TREE_ADDRESSABLE (expr) = 1; tree ret = build_fold_addr_expr_loc(location.gcc_location(), expr); return this->make_expression(ret); } -- 2.17.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] go/100537 - Bootstrap-O3 and bootstrap-debug fail 2021-05-14 2:52 [PATCH] go/100537 - Bootstrap-O3 and bootstrap-debug fail Jiufu Guo @ 2021-05-14 7:15 ` Richard Biener 2021-05-14 7:39 ` guojiufu 0 siblings, 1 reply; 11+ messages in thread From: Richard Biener @ 2021-05-14 7:15 UTC (permalink / raw) To: Jiufu Guo, gcc-patches; +Cc: guojiufu, wschmidt, ian On May 14, 2021 4:52:56 AM GMT+02:00, Jiufu Guo <guojiufu@linux.ibm.com> wrote: >As discussed in the PR, Richard mentioned the method to >figure out which VAR was not set TREE_ADDRESSABLE, and >then cause this failure. It is address_expression which >build addr_expr (build_fold_addr_expr_loc), but not set >TREE_ADDRESSABLE. > >I drafted this patch with reference the comments from Richard >in this PR, while I'm not quite sure if more thing need to do. >So, please have review, thanks! > >Bootstrap and regtest pass on ppc64le. Is this ok for trunk? I suggest to use mark_addresssable unless we're sure expr is always an entity where TREE_ADDRESSABLE has the desired meaning. Richard. >Jiufu Guo. > >2021-05-14 Richard Biener <rguenther@suse.de> > Jiufu Guo <guojiufu@linux.ibm.com> > > PR go/100537 > * go-gcc.cc > (Gcc_backend::address_expression): Set TREE_ADDRESSABLE. > >--- > gcc/go/go-gcc.cc | 1 + > 1 file changed, 1 insertion(+) > >diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc >index 5d9dbb5d068..8ed20a3b479 100644 >--- a/gcc/go/go-gcc.cc >+++ b/gcc/go/go-gcc.cc >@@ -1680,6 +1680,7 @@ Gcc_backend::address_expression(Bexpression* >bexpr, Location location) > if (expr == error_mark_node) > return this->error_expression(); > >+ TREE_ADDRESSABLE (expr) = 1; > tree ret = build_fold_addr_expr_loc(location.gcc_location(), expr); > return this->make_expression(ret); > } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] go/100537 - Bootstrap-O3 and bootstrap-debug fail 2021-05-14 7:15 ` Richard Biener @ 2021-05-14 7:39 ` guojiufu 2021-05-14 8:21 ` guojiufu 0 siblings, 1 reply; 11+ messages in thread From: guojiufu @ 2021-05-14 7:39 UTC (permalink / raw) To: Richard Biener; +Cc: gcc-patches, wschmidt, ian On 2021-05-14 15:15, Richard Biener wrote: > On May 14, 2021 4:52:56 AM GMT+02:00, Jiufu Guo > <guojiufu@linux.ibm.com> wrote: >> As discussed in the PR, Richard mentioned the method to >> figure out which VAR was not set TREE_ADDRESSABLE, and >> then cause this failure. It is address_expression which >> build addr_expr (build_fold_addr_expr_loc), but not set >> TREE_ADDRESSABLE. >> >> I drafted this patch with reference the comments from Richard >> in this PR, while I'm not quite sure if more thing need to do. >> So, please have review, thanks! >> >> Bootstrap and regtest pass on ppc64le. Is this ok for trunk? > > I suggest to use mark_addresssable unless we're sure expr is always an > entity where TREE_ADDRESSABLE has the desired meaning. I notice you mentioned "mark_addresssable" in PR. And I had tried yesterday, it cause new ICEs at gimple-expr.c:918 below line: && cfun->gimple_df != NULL > > Richard. > >> Jiufu Guo. >> >> 2021-05-14 Richard Biener <rguenther@suse.de> >> Jiufu Guo <guojiufu@linux.ibm.com> >> >> PR go/100537 >> * go-gcc.cc >> (Gcc_backend::address_expression): Set TREE_ADDRESSABLE. >> >> --- >> gcc/go/go-gcc.cc | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc >> index 5d9dbb5d068..8ed20a3b479 100644 >> --- a/gcc/go/go-gcc.cc >> +++ b/gcc/go/go-gcc.cc >> @@ -1680,6 +1680,7 @@ Gcc_backend::address_expression(Bexpression* >> bexpr, Location location) >> if (expr == error_mark_node) >> return this->error_expression(); >> >> + TREE_ADDRESSABLE (expr) = 1; >> tree ret = build_fold_addr_expr_loc(location.gcc_location(), expr); >> return this->make_expression(ret); >> } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] go/100537 - Bootstrap-O3 and bootstrap-debug fail 2021-05-14 7:39 ` guojiufu @ 2021-05-14 8:21 ` guojiufu 2021-05-17 8:17 ` Richard Biener 0 siblings, 1 reply; 11+ messages in thread From: guojiufu @ 2021-05-14 8:21 UTC (permalink / raw) To: guojiufu; +Cc: Richard Biener, wschmidt, ian, gcc-patches On 2021-05-14 15:39, guojiufu via Gcc-patches wrote: > On 2021-05-14 15:15, Richard Biener wrote: >> On May 14, 2021 4:52:56 AM GMT+02:00, Jiufu Guo >> <guojiufu@linux.ibm.com> wrote: >>> As discussed in the PR, Richard mentioned the method to >>> figure out which VAR was not set TREE_ADDRESSABLE, and >>> then cause this failure. It is address_expression which >>> build addr_expr (build_fold_addr_expr_loc), but not set >>> TREE_ADDRESSABLE. >>> >>> I drafted this patch with reference the comments from Richard >>> in this PR, while I'm not quite sure if more thing need to do. >>> So, please have review, thanks! >>> >>> Bootstrap and regtest pass on ppc64le. Is this ok for trunk? >> >> I suggest to use mark_addresssable unless we're sure expr is always an >> entity where TREE_ADDRESSABLE has the desired meaning. Thanks, Richard! You point out the root concern, I'm not sure ;) With looking at code "mark_addresssable" and code around tree-ssa.c:1013, VAR_P, PARM_DECL, and RESULT_DECL are checked before accessing TREE_ADDRESSABLE. So, just wondering if these entities need to be marked as TREE_ADDRESSABLE? diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc index 5d9dbb5d068..85d324a92cc 100644 --- a/gcc/go/go-gcc.cc +++ b/gcc/go/go-gcc.cc @@ -1680,6 +1680,11 @@ Gcc_backend::address_expression(Bexpression* bexpr, Location location) if (expr == error_mark_node) return this->error_expression(); + if ((VAR_P(expr) + || TREE_CODE(expr) == PARM_DECL + || TREE_CODE(expr) == RESULT_DECL) + TREE_ADDRESSABLE (expr) = 1; + tree ret = build_fold_addr_expr_loc(location.gcc_location(), expr); return this->make_expression(ret); } Or call mark_addressable, and update mark_addressable to avoid NULL pointer ICE: The below patch also pass bootstrap-debug. diff --git a/gcc/gimple-expr.c b/gcc/gimple-expr.c index b8c732b632a..f682841391b 100644 --- a/gcc/gimple-expr.c +++ b/gcc/gimple-expr.c @@ -915,6 +915,7 @@ mark_addressable (tree x) if (TREE_CODE (x) == VAR_DECL && !DECL_EXTERNAL (x) && !TREE_STATIC (x) + && cfun != NULL && cfun->gimple_df != NULL && cfun->gimple_df->decls_to_pointers != NULL) { diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc index 5d9dbb5d068..fe9dfaf8579 100644 --- a/gcc/go/go-gcc.cc +++ b/gcc/go/go-gcc.cc @@ -1680,6 +1680,7 @@ Gcc_backend::address_expression(Bexpression* bexpr, Location location) if (expr == error_mark_node) return this->error_expression(); + mark_addressable(expr); tree ret = build_fold_addr_expr_loc(location.gcc_location(), expr); return this->make_expression(ret); } > > I notice you mentioned "mark_addresssable" in PR. > And I had tried yesterday, it cause new ICEs at gimple-expr.c:918 > below line: > > && cfun->gimple_df != NULL > > > >> >> Richard. >> >>> Jiufu Guo. >>> >>> 2021-05-14 Richard Biener <rguenther@suse.de> >>> Jiufu Guo <guojiufu@linux.ibm.com> >>> >>> PR go/100537 >>> * go-gcc.cc >>> (Gcc_backend::address_expression): Set TREE_ADDRESSABLE. >>> >>> --- >>> gcc/go/go-gcc.cc | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc >>> index 5d9dbb5d068..8ed20a3b479 100644 >>> --- a/gcc/go/go-gcc.cc >>> +++ b/gcc/go/go-gcc.cc >>> @@ -1680,6 +1680,7 @@ Gcc_backend::address_expression(Bexpression* >>> bexpr, Location location) >>> if (expr == error_mark_node) >>> return this->error_expression(); >>> >>> + TREE_ADDRESSABLE (expr) = 1; >>> tree ret = build_fold_addr_expr_loc(location.gcc_location(), expr); >>> return this->make_expression(ret); >>> } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] go/100537 - Bootstrap-O3 and bootstrap-debug fail 2021-05-14 8:21 ` guojiufu @ 2021-05-17 8:17 ` Richard Biener 2021-05-18 1:18 ` guojiufu 2021-05-18 1:48 ` Ian Lance Taylor 0 siblings, 2 replies; 11+ messages in thread From: Richard Biener @ 2021-05-17 8:17 UTC (permalink / raw) To: guojiufu; +Cc: Bill Schmidt, Ian Lance Taylor, Richard Biener, GCC Patches On Fri, May 14, 2021 at 11:19 AM guojiufu via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > On 2021-05-14 15:39, guojiufu via Gcc-patches wrote: > > On 2021-05-14 15:15, Richard Biener wrote: > >> On May 14, 2021 4:52:56 AM GMT+02:00, Jiufu Guo > >> <guojiufu@linux.ibm.com> wrote: > >>> As discussed in the PR, Richard mentioned the method to > >>> figure out which VAR was not set TREE_ADDRESSABLE, and > >>> then cause this failure. It is address_expression which > >>> build addr_expr (build_fold_addr_expr_loc), but not set > >>> TREE_ADDRESSABLE. > >>> > >>> I drafted this patch with reference the comments from Richard > >>> in this PR, while I'm not quite sure if more thing need to do. > >>> So, please have review, thanks! > >>> > >>> Bootstrap and regtest pass on ppc64le. Is this ok for trunk? > >> > >> I suggest to use mark_addresssable unless we're sure expr is always an > >> entity where TREE_ADDRESSABLE has the desired meaning. > > Thanks, Richard! > You point out the root concern, I'm not sure ;) > > With looking at code "mark_addresssable" and code around > tree-ssa.c:1013, > VAR_P, PARM_DECL, and RESULT_DECL are checked before accessing > TREE_ADDRESSABLE. > So, just wondering if these entities need to be marked as > TREE_ADDRESSABLE? > > diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc > index 5d9dbb5d068..85d324a92cc 100644 > --- a/gcc/go/go-gcc.cc > +++ b/gcc/go/go-gcc.cc > @@ -1680,6 +1680,11 @@ Gcc_backend::address_expression(Bexpression* > bexpr, Location location) > if (expr == error_mark_node) > return this->error_expression(); > > + if ((VAR_P(expr) > + || TREE_CODE(expr) == PARM_DECL > + || TREE_CODE(expr) == RESULT_DECL) > + TREE_ADDRESSABLE (expr) = 1; > + The root concern is that mark_addressable does while (handled_component_p (x)) x = TREE_OPERAND (x, 0); and I do not know the constraints on 'expr' as passed to Gcc_backend::address_expression. I think we need input from Ian here. Most FEs have their own *_mark_addressable function where they also emit diagnostics (guess this is handled in the actual Go frontend). Since Gcc_backend does lowering to GENERIC using a middle-end is probably OK. > tree ret = build_fold_addr_expr_loc(location.gcc_location(), expr); > return this->make_expression(ret); > } > > > Or call mark_addressable, and update mark_addressable to avoid NULL > pointer ICE: > The below patch also pass bootstrap-debug. > > diff --git a/gcc/gimple-expr.c b/gcc/gimple-expr.c > index b8c732b632a..f682841391b 100644 > --- a/gcc/gimple-expr.c > +++ b/gcc/gimple-expr.c > @@ -915,6 +915,7 @@ mark_addressable (tree x) > if (TREE_CODE (x) == VAR_DECL > && !DECL_EXTERNAL (x) > && !TREE_STATIC (x) > + && cfun != NULL I'd be OK with this hunk of course. > && cfun->gimple_df != NULL > && cfun->gimple_df->decls_to_pointers != NULL) > { > diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc > index 5d9dbb5d068..fe9dfaf8579 100644 > --- a/gcc/go/go-gcc.cc > +++ b/gcc/go/go-gcc.cc > @@ -1680,6 +1680,7 @@ Gcc_backend::address_expression(Bexpression* > bexpr, Location location) > if (expr == error_mark_node) > return this->error_expression(); > > + mark_addressable(expr); > tree ret = build_fold_addr_expr_loc(location.gcc_location(), expr); > return this->make_expression(ret); > } > > > > > > I notice you mentioned "mark_addresssable" in PR. > > And I had tried yesterday, it cause new ICEs at gimple-expr.c:918 > > below line: > > > > && cfun->gimple_df != NULL > > > > > > > >> > >> Richard. > >> > >>> Jiufu Guo. > >>> > >>> 2021-05-14 Richard Biener <rguenther@suse.de> > >>> Jiufu Guo <guojiufu@linux.ibm.com> > >>> > >>> PR go/100537 > >>> * go-gcc.cc > >>> (Gcc_backend::address_expression): Set TREE_ADDRESSABLE. > >>> > >>> --- > >>> gcc/go/go-gcc.cc | 1 + > >>> 1 file changed, 1 insertion(+) > >>> > >>> diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc > >>> index 5d9dbb5d068..8ed20a3b479 100644 > >>> --- a/gcc/go/go-gcc.cc > >>> +++ b/gcc/go/go-gcc.cc > >>> @@ -1680,6 +1680,7 @@ Gcc_backend::address_expression(Bexpression* > >>> bexpr, Location location) > >>> if (expr == error_mark_node) > >>> return this->error_expression(); > >>> > >>> + TREE_ADDRESSABLE (expr) = 1; > >>> tree ret = build_fold_addr_expr_loc(location.gcc_location(), expr); > >>> return this->make_expression(ret); > >>> } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] go/100537 - Bootstrap-O3 and bootstrap-debug fail 2021-05-17 8:17 ` Richard Biener @ 2021-05-18 1:18 ` guojiufu 2021-05-18 1:48 ` Ian Lance Taylor 1 sibling, 0 replies; 11+ messages in thread From: guojiufu @ 2021-05-18 1:18 UTC (permalink / raw) To: Richard Biener Cc: Bill Schmidt, Ian Lance Taylor, Richard Biener, GCC Patches On 2021-05-17 16:17, Richard Biener wrote: > On Fri, May 14, 2021 at 11:19 AM guojiufu via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: >> >> On 2021-05-14 15:39, guojiufu via Gcc-patches wrote: >> > On 2021-05-14 15:15, Richard Biener wrote: >> >> On May 14, 2021 4:52:56 AM GMT+02:00, Jiufu Guo >> >> <guojiufu@linux.ibm.com> wrote: >> >>> As discussed in the PR, Richard mentioned the method to >> >>> figure out which VAR was not set TREE_ADDRESSABLE, and >> >>> then cause this failure. It is address_expression which >> >>> build addr_expr (build_fold_addr_expr_loc), but not set >> >>> TREE_ADDRESSABLE. >> >>> >> >>> I drafted this patch with reference the comments from Richard >> >>> in this PR, while I'm not quite sure if more thing need to do. >> >>> So, please have review, thanks! >> >>> >> >>> Bootstrap and regtest pass on ppc64le. Is this ok for trunk? >> >> >> >> I suggest to use mark_addresssable unless we're sure expr is always an >> >> entity where TREE_ADDRESSABLE has the desired meaning. >> >> Thanks, Richard! >> You point out the root concern, I'm not sure ;) >> >> With looking at code "mark_addresssable" and code around >> tree-ssa.c:1013, >> VAR_P, PARM_DECL, and RESULT_DECL are checked before accessing >> TREE_ADDRESSABLE. >> So, just wondering if these entities need to be marked as >> TREE_ADDRESSABLE? >> >> diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc >> index 5d9dbb5d068..85d324a92cc 100644 >> --- a/gcc/go/go-gcc.cc >> +++ b/gcc/go/go-gcc.cc >> @@ -1680,6 +1680,11 @@ Gcc_backend::address_expression(Bexpression* >> bexpr, Location location) >> if (expr == error_mark_node) >> return this->error_expression(); >> >> + if ((VAR_P(expr) >> + || TREE_CODE(expr) == PARM_DECL >> + || TREE_CODE(expr) == RESULT_DECL) >> + TREE_ADDRESSABLE (expr) = 1; >> + > > The root concern is that mark_addressable does > > while (handled_component_p (x)) > x = TREE_OPERAND (x, 0); > > and I do not know the constraints on 'expr' as passed to > Gcc_backend::address_expression. > > I think we need input from Ian here. Most FEs have their own > *_mark_addressable > function where they also emit diagnostics (guess this is handled in > the actual Go frontend). > Since Gcc_backend does lowering to GENERIC using a middle-end is > probably OK. Yeap. Hope this patch is ok, then the bootstrap could pass. Otherwise, we may need more help from Ian and guys ;) Jiufu Guo. > >> tree ret = build_fold_addr_expr_loc(location.gcc_location(), >> expr); >> return this->make_expression(ret); >> } >> >> >> Or call mark_addressable, and update mark_addressable to avoid NULL >> pointer ICE: >> The below patch also pass bootstrap-debug. >> >> diff --git a/gcc/gimple-expr.c b/gcc/gimple-expr.c >> index b8c732b632a..f682841391b 100644 >> --- a/gcc/gimple-expr.c >> +++ b/gcc/gimple-expr.c >> @@ -915,6 +915,7 @@ mark_addressable (tree x) >> if (TREE_CODE (x) == VAR_DECL >> && !DECL_EXTERNAL (x) >> && !TREE_STATIC (x) >> + && cfun != NULL > > I'd be OK with this hunk of course. > >> && cfun->gimple_df != NULL >> && cfun->gimple_df->decls_to_pointers != NULL) >> { >> diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc >> index 5d9dbb5d068..fe9dfaf8579 100644 >> --- a/gcc/go/go-gcc.cc >> +++ b/gcc/go/go-gcc.cc >> @@ -1680,6 +1680,7 @@ Gcc_backend::address_expression(Bexpression* >> bexpr, Location location) >> if (expr == error_mark_node) >> return this->error_expression(); >> >> + mark_addressable(expr); >> tree ret = build_fold_addr_expr_loc(location.gcc_location(), >> expr); >> return this->make_expression(ret); >> } >> >> >> > >> > I notice you mentioned "mark_addresssable" in PR. >> > And I had tried yesterday, it cause new ICEs at gimple-expr.c:918 >> > below line: >> > >> > && cfun->gimple_df != NULL >> > >> > >> > >> >> >> >> Richard. >> >> >> >>> Jiufu Guo. >> >>> >> >>> 2021-05-14 Richard Biener <rguenther@suse.de> >> >>> Jiufu Guo <guojiufu@linux.ibm.com> >> >>> >> >>> PR go/100537 >> >>> * go-gcc.cc >> >>> (Gcc_backend::address_expression): Set TREE_ADDRESSABLE. >> >>> >> >>> --- >> >>> gcc/go/go-gcc.cc | 1 + >> >>> 1 file changed, 1 insertion(+) >> >>> >> >>> diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc >> >>> index 5d9dbb5d068..8ed20a3b479 100644 >> >>> --- a/gcc/go/go-gcc.cc >> >>> +++ b/gcc/go/go-gcc.cc >> >>> @@ -1680,6 +1680,7 @@ Gcc_backend::address_expression(Bexpression* >> >>> bexpr, Location location) >> >>> if (expr == error_mark_node) >> >>> return this->error_expression(); >> >>> >> >>> + TREE_ADDRESSABLE (expr) = 1; >> >>> tree ret = build_fold_addr_expr_loc(location.gcc_location(), expr); >> >>> return this->make_expression(ret); >> >>> } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] go/100537 - Bootstrap-O3 and bootstrap-debug fail 2021-05-17 8:17 ` Richard Biener 2021-05-18 1:18 ` guojiufu @ 2021-05-18 1:48 ` Ian Lance Taylor 2021-05-18 6:58 ` Richard Biener 1 sibling, 1 reply; 11+ messages in thread From: Ian Lance Taylor @ 2021-05-18 1:48 UTC (permalink / raw) To: Richard Biener Cc: guojiufu, Richard Biener, Ian Lance Taylor, Bill Schmidt, GCC Patches On Mon, May 17, 2021 at 1:17 AM Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > On Fri, May 14, 2021 at 11:19 AM guojiufu via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: > > > > On 2021-05-14 15:39, guojiufu via Gcc-patches wrote: > > > On 2021-05-14 15:15, Richard Biener wrote: > > >> On May 14, 2021 4:52:56 AM GMT+02:00, Jiufu Guo > > >> <guojiufu@linux.ibm.com> wrote: > > >>> As discussed in the PR, Richard mentioned the method to > > >>> figure out which VAR was not set TREE_ADDRESSABLE, and > > >>> then cause this failure. It is address_expression which > > >>> build addr_expr (build_fold_addr_expr_loc), but not set > > >>> TREE_ADDRESSABLE. > > >>> > > >>> I drafted this patch with reference the comments from Richard > > >>> in this PR, while I'm not quite sure if more thing need to do. > > >>> So, please have review, thanks! > > >>> > > >>> Bootstrap and regtest pass on ppc64le. Is this ok for trunk? > > >> > > >> I suggest to use mark_addresssable unless we're sure expr is always an > > >> entity where TREE_ADDRESSABLE has the desired meaning. > > > > Thanks, Richard! > > You point out the root concern, I'm not sure ;) > > > > With looking at code "mark_addresssable" and code around > > tree-ssa.c:1013, > > VAR_P, PARM_DECL, and RESULT_DECL are checked before accessing > > TREE_ADDRESSABLE. > > So, just wondering if these entities need to be marked as > > TREE_ADDRESSABLE? > > > > diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc > > index 5d9dbb5d068..85d324a92cc 100644 > > --- a/gcc/go/go-gcc.cc > > +++ b/gcc/go/go-gcc.cc > > @@ -1680,6 +1680,11 @@ Gcc_backend::address_expression(Bexpression* > > bexpr, Location location) > > if (expr == error_mark_node) > > return this->error_expression(); > > > > + if ((VAR_P(expr) > > + || TREE_CODE(expr) == PARM_DECL > > + || TREE_CODE(expr) == RESULT_DECL) > > + TREE_ADDRESSABLE (expr) = 1; > > + > > The root concern is that mark_addressable does > > while (handled_component_p (x)) > x = TREE_OPERAND (x, 0); > > and I do not know the constraints on 'expr' as passed to > Gcc_backend::address_expression. > > I think we need input from Ian here. Most FEs have their own *_mark_addressable > function where they also emit diagnostics (guess this is handled in > the actual Go frontend). > Since Gcc_backend does lowering to GENERIC using a middle-end is probably OK. I doubt I understand all the issues here. In general the Go frontend only takes the addresses of VAR_DECLs or PARM_DECLs. It doesn't bother to set TREE_ADDRESSABLE for global variables for which TREE_STATIC or DECL_EXTERNAL is true. For local variables it sets TREE_ADDRESSABLE based on the is_address_taken parameter to Gcc_backend::local_variable, and similarly for PARM_DECLs and Gcc_backend::parameter_variable. The name in the bug report is for a string initializer, which should be TREE_STATIC == 1 and TREE_PUBLIC == 0. Perhaps the fix is simply to set TREE_ADDRESSABLE in Gcc_backend::immutable_struct and Gcc_backend::implicit_variable. I can't see how it would hurt to set TREE_ADDRESSABLE unnecessarily for a TREE_STATIC variable. But, again, I doubt I understand all the issues here. Ian ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] go/100537 - Bootstrap-O3 and bootstrap-debug fail 2021-05-18 1:48 ` Ian Lance Taylor @ 2021-05-18 6:58 ` Richard Biener 2021-05-18 13:14 ` guojiufu 2021-05-20 8:59 ` guojiufu 0 siblings, 2 replies; 11+ messages in thread From: Richard Biener @ 2021-05-18 6:58 UTC (permalink / raw) To: Ian Lance Taylor Cc: Richard Biener, guojiufu, Ian Lance Taylor, Bill Schmidt, GCC Patches On Mon, 17 May 2021, Ian Lance Taylor wrote: > On Mon, May 17, 2021 at 1:17 AM Richard Biener via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: > > > > On Fri, May 14, 2021 at 11:19 AM guojiufu via Gcc-patches > > <gcc-patches@gcc.gnu.org> wrote: > > > > > > On 2021-05-14 15:39, guojiufu via Gcc-patches wrote: > > > > On 2021-05-14 15:15, Richard Biener wrote: > > > >> On May 14, 2021 4:52:56 AM GMT+02:00, Jiufu Guo > > > >> <guojiufu@linux.ibm.com> wrote: > > > >>> As discussed in the PR, Richard mentioned the method to > > > >>> figure out which VAR was not set TREE_ADDRESSABLE, and > > > >>> then cause this failure. It is address_expression which > > > >>> build addr_expr (build_fold_addr_expr_loc), but not set > > > >>> TREE_ADDRESSABLE. > > > >>> > > > >>> I drafted this patch with reference the comments from Richard > > > >>> in this PR, while I'm not quite sure if more thing need to do. > > > >>> So, please have review, thanks! > > > >>> > > > >>> Bootstrap and regtest pass on ppc64le. Is this ok for trunk? > > > >> > > > >> I suggest to use mark_addresssable unless we're sure expr is always an > > > >> entity where TREE_ADDRESSABLE has the desired meaning. > > > > > > Thanks, Richard! > > > You point out the root concern, I'm not sure ;) > > > > > > With looking at code "mark_addresssable" and code around > > > tree-ssa.c:1013, > > > VAR_P, PARM_DECL, and RESULT_DECL are checked before accessing > > > TREE_ADDRESSABLE. > > > So, just wondering if these entities need to be marked as > > > TREE_ADDRESSABLE? > > > > > > diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc > > > index 5d9dbb5d068..85d324a92cc 100644 > > > --- a/gcc/go/go-gcc.cc > > > +++ b/gcc/go/go-gcc.cc > > > @@ -1680,6 +1680,11 @@ Gcc_backend::address_expression(Bexpression* > > > bexpr, Location location) > > > if (expr == error_mark_node) > > > return this->error_expression(); > > > > > > + if ((VAR_P(expr) > > > + || TREE_CODE(expr) == PARM_DECL > > > + || TREE_CODE(expr) == RESULT_DECL) > > > + TREE_ADDRESSABLE (expr) = 1; > > > + > > > > The root concern is that mark_addressable does > > > > while (handled_component_p (x)) > > x = TREE_OPERAND (x, 0); > > > > and I do not know the constraints on 'expr' as passed to > > Gcc_backend::address_expression. > > > > I think we need input from Ian here. Most FEs have their own *_mark_addressable > > function where they also emit diagnostics (guess this is handled in > > the actual Go frontend). > > Since Gcc_backend does lowering to GENERIC using a middle-end is probably OK. > > I doubt I understand all the issues here. > > In general the Go frontend only takes the addresses of VAR_DECLs or > PARM_DECLs. It doesn't bother to set TREE_ADDRESSABLE for global > variables for which TREE_STATIC or DECL_EXTERNAL is true. For local > variables it sets TREE_ADDRESSABLE based on the is_address_taken > parameter to Gcc_backend::local_variable, and similarly for PARM_DECLs > and Gcc_backend::parameter_variable. > > The name in the bug report is for a string initializer, which should > be TREE_STATIC == 1 and TREE_PUBLIC == 0. Perhaps the fix is simply > to set TREE_ADDRESSABLE in Gcc_backend::immutable_struct and > Gcc_backend::implicit_variable. I can't see how it would hurt to set > TREE_ADDRESSABLE unnecessarily for a TREE_STATIC variable. > > But, again, I doubt I understand all the issues here. GENERIC requires TREE_ADDRESSABLE to be set on all address-taken VAR_DECLs, PARM_DECLs and RESULT_DECLs - the gimplifier is the first to require this for correctness. Setting TREE_ADDRESSABLE when the address is not taken is harmless and at most results in missed optimizations (on most entities we are able to clear the flag later). We're currently quite forgiving with this though (still the gimplifier can generate wrong-code). The trigger of the current failure removed one "forgiveness", I do plan to remove a few more. guojiufu's patch works for me but as said I'm not sure if there's a better place to set TREE_ADDRESSABLE for entities that have their address taken - definitely catching the places where you build an ADDR_EXPR are the most obvious ones. Richard. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] go/100537 - Bootstrap-O3 and bootstrap-debug fail 2021-05-18 6:58 ` Richard Biener @ 2021-05-18 13:14 ` guojiufu 2021-05-20 8:59 ` guojiufu 1 sibling, 0 replies; 11+ messages in thread From: guojiufu @ 2021-05-18 13:14 UTC (permalink / raw) To: Richard Biener Cc: Ian Lance Taylor, Richard Biener, Ian Lance Taylor, Bill Schmidt, GCC Patches On 2021-05-18 14:58, Richard Biener wrote: > On Mon, 17 May 2021, Ian Lance Taylor wrote: > >> On Mon, May 17, 2021 at 1:17 AM Richard Biener via Gcc-patches >> <gcc-patches@gcc.gnu.org> wrote: >> > >> > On Fri, May 14, 2021 at 11:19 AM guojiufu via Gcc-patches >> > <gcc-patches@gcc.gnu.org> wrote: >> > > >> > > On 2021-05-14 15:39, guojiufu via Gcc-patches wrote: >> > > > On 2021-05-14 15:15, Richard Biener wrote: >> > > >> On May 14, 2021 4:52:56 AM GMT+02:00, Jiufu Guo >> > > >> <guojiufu@linux.ibm.com> wrote: >> > > >>> As discussed in the PR, Richard mentioned the method to >> > > >>> figure out which VAR was not set TREE_ADDRESSABLE, and >> > > >>> then cause this failure. It is address_expression which >> > > >>> build addr_expr (build_fold_addr_expr_loc), but not set >> > > >>> TREE_ADDRESSABLE. >> > > >>> >> > > >>> I drafted this patch with reference the comments from Richard >> > > >>> in this PR, while I'm not quite sure if more thing need to do. >> > > >>> So, please have review, thanks! >> > > >>> >> > > >>> Bootstrap and regtest pass on ppc64le. Is this ok for trunk? >> > > >> >> > > >> I suggest to use mark_addresssable unless we're sure expr is always an >> > > >> entity where TREE_ADDRESSABLE has the desired meaning. >> > > >> > > Thanks, Richard! >> > > You point out the root concern, I'm not sure ;) >> > > >> > > With looking at code "mark_addresssable" and code around >> > > tree-ssa.c:1013, >> > > VAR_P, PARM_DECL, and RESULT_DECL are checked before accessing >> > > TREE_ADDRESSABLE. >> > > So, just wondering if these entities need to be marked as >> > > TREE_ADDRESSABLE? >> > > >> > > diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc >> > > index 5d9dbb5d068..85d324a92cc 100644 >> > > --- a/gcc/go/go-gcc.cc >> > > +++ b/gcc/go/go-gcc.cc >> > > @@ -1680,6 +1680,11 @@ Gcc_backend::address_expression(Bexpression* >> > > bexpr, Location location) >> > > if (expr == error_mark_node) >> > > return this->error_expression(); >> > > >> > > + if ((VAR_P(expr) >> > > + || TREE_CODE(expr) == PARM_DECL >> > > + || TREE_CODE(expr) == RESULT_DECL) >> > > + TREE_ADDRESSABLE (expr) = 1; >> > > + >> > >> > The root concern is that mark_addressable does >> > >> > while (handled_component_p (x)) >> > x = TREE_OPERAND (x, 0); >> > >> > and I do not know the constraints on 'expr' as passed to >> > Gcc_backend::address_expression. >> > >> > I think we need input from Ian here. Most FEs have their own *_mark_addressable >> > function where they also emit diagnostics (guess this is handled in >> > the actual Go frontend). >> > Since Gcc_backend does lowering to GENERIC using a middle-end is probably OK. >> >> I doubt I understand all the issues here. >> >> In general the Go frontend only takes the addresses of VAR_DECLs or >> PARM_DECLs. It doesn't bother to set TREE_ADDRESSABLE for global >> variables for which TREE_STATIC or DECL_EXTERNAL is true. For local >> variables it sets TREE_ADDRESSABLE based on the is_address_taken >> parameter to Gcc_backend::local_variable, and similarly for PARM_DECLs >> and Gcc_backend::parameter_variable. >> >> The name in the bug report is for a string initializer, which should >> be TREE_STATIC == 1 and TREE_PUBLIC == 0. Perhaps the fix is simply >> to set TREE_ADDRESSABLE in Gcc_backend::immutable_struct and >> Gcc_backend::implicit_variable. I can't see how it would hurt to set >> TREE_ADDRESSABLE unnecessarily for a TREE_STATIC variable. One more finding: Gcc_backend::implicit_variable -> build_decl is called for "<var_decl 0x200000a285e0 go..C479>" at Unary_expression::do_get_backend (expressions.cc:5322). And, this code (as below) from "expressions.cc:5322" Unary_expression::do_get_backend (expressions.cc:5322): gogo->backend()->implicit_variable(var_name, "", btype, true, true, false, 0); where var_name is go..C479 This code is under **"case OPERATOR_AND:"** of a switch statement. Unary_expression with OPERATOR_AND is "&" expression, I guess, it may look as taking address. And as the log mentioned: "PHI <err$__object_76(58), &go..C479(59)>". In this phi, &go..C479(59) would be the Unary_expression with OPERATOR_AND on "go..C479". So, I guess, we may able to treat "Unary_expression with OPERATOR_AND" as address_taken operation. Then it would be necessary to mark addressable. address_expression: "ret = gogo->backend()->address_expression(bexpr, loc);" (expressions.cc:5330) is already called under "Unary_expression with OPERATOR_AND". Does this make sense? If so, we may set "TREE_ADDRESSABLE" just before expressions.cc:5330? Hope this finding is helpful. BR. Jiufu Guo. >> >> But, again, I doubt I understand all the issues here. > > GENERIC requires TREE_ADDRESSABLE to be set on all address-taken > VAR_DECLs, PARM_DECLs and RESULT_DECLs - the gimplifier is the > first to require this for correctness. Setting TREE_ADDRESSABLE > when the address is not taken is harmless and at most results in > missed optimizations (on most entities we are able to clear the > flag later). > > We're currently quite forgiving with this though (still the > gimplifier can generate wrong-code). The trigger of the current > failure removed one "forgiveness", I do plan to remove a few more. > > guojiufu's patch works for me but as said I'm not sure if there's > a better place to set TREE_ADDRESSABLE for entities that have > their address taken - definitely catching the places where > you build an ADDR_EXPR are the most obvious ones. > > Richard. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] go/100537 - Bootstrap-O3 and bootstrap-debug fail 2021-05-18 6:58 ` Richard Biener 2021-05-18 13:14 ` guojiufu @ 2021-05-20 8:59 ` guojiufu 2021-05-24 21:46 ` Ian Lance Taylor 1 sibling, 1 reply; 11+ messages in thread From: guojiufu @ 2021-05-20 8:59 UTC (permalink / raw) To: Richard Biener Cc: Ian Lance Taylor, Richard Biener, Ian Lance Taylor, Bill Schmidt, GCC Patches On 2021-05-18 14:58, Richard Biener wrote: > On Mon, 17 May 2021, Ian Lance Taylor wrote: > >> On Mon, May 17, 2021 at 1:17 AM Richard Biener via Gcc-patches >> <gcc-patches@gcc.gnu.org> wrote: >> > >> > On Fri, May 14, 2021 at 11:19 AM guojiufu via Gcc-patches >> > <gcc-patches@gcc.gnu.org> wrote: >> > > >> > > On 2021-05-14 15:39, guojiufu via Gcc-patches wrote: >> > > > On 2021-05-14 15:15, Richard Biener wrote: >> > > >> On May 14, 2021 4:52:56 AM GMT+02:00, Jiufu Guo >> > > >> <guojiufu@linux.ibm.com> wrote: >> > > >>> As discussed in the PR, Richard mentioned the method to >> > > >>> figure out which VAR was not set TREE_ADDRESSABLE, and >> > > >>> then cause this failure. It is address_expression which >> > > >>> build addr_expr (build_fold_addr_expr_loc), but not set >> > > >>> TREE_ADDRESSABLE. >> > > >>> >> > > >>> I drafted this patch with reference the comments from Richard >> > > >>> in this PR, while I'm not quite sure if more thing need to do. >> > > >>> So, please have review, thanks! >> > > >>> >> > > >>> Bootstrap and regtest pass on ppc64le. Is this ok for trunk? >> > > >> >> > > >> I suggest to use mark_addresssable unless we're sure expr is always an >> > > >> entity where TREE_ADDRESSABLE has the desired meaning. >> > > >> > > Thanks, Richard! >> > > You point out the root concern, I'm not sure ;) >> > > >> > > With looking at code "mark_addresssable" and code around >> > > tree-ssa.c:1013, >> > > VAR_P, PARM_DECL, and RESULT_DECL are checked before accessing >> > > TREE_ADDRESSABLE. >> > > So, just wondering if these entities need to be marked as >> > > TREE_ADDRESSABLE? >> > > >> > > diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc >> > > index 5d9dbb5d068..85d324a92cc 100644 >> > > --- a/gcc/go/go-gcc.cc >> > > +++ b/gcc/go/go-gcc.cc >> > > @@ -1680,6 +1680,11 @@ Gcc_backend::address_expression(Bexpression* >> > > bexpr, Location location) >> > > if (expr == error_mark_node) >> > > return this->error_expression(); >> > > >> > > + if ((VAR_P(expr) >> > > + || TREE_CODE(expr) == PARM_DECL >> > > + || TREE_CODE(expr) == RESULT_DECL) >> > > + TREE_ADDRESSABLE (expr) = 1; >> > > + >> > >> > The root concern is that mark_addressable does >> > >> > while (handled_component_p (x)) >> > x = TREE_OPERAND (x, 0); >> > >> > and I do not know the constraints on 'expr' as passed to >> > Gcc_backend::address_expression. >> > >> > I think we need input from Ian here. Most FEs have their own *_mark_addressable >> > function where they also emit diagnostics (guess this is handled in >> > the actual Go frontend). >> > Since Gcc_backend does lowering to GENERIC using a middle-end is probably OK. >> >> I doubt I understand all the issues here. >> >> In general the Go frontend only takes the addresses of VAR_DECLs or >> PARM_DECLs. It doesn't bother to set TREE_ADDRESSABLE for global >> variables for which TREE_STATIC or DECL_EXTERNAL is true. For local >> variables it sets TREE_ADDRESSABLE based on the is_address_taken >> parameter to Gcc_backend::local_variable, and similarly for PARM_DECLs >> and Gcc_backend::parameter_variable. >> >> The name in the bug report is for a string initializer, which should >> be TREE_STATIC == 1 and TREE_PUBLIC == 0. Perhaps the fix is simply >> to set TREE_ADDRESSABLE in Gcc_backend::immutable_struct and >> Gcc_backend::implicit_variable. I can't see how it would hurt to set >> TREE_ADDRESSABLE unnecessarily for a TREE_STATIC variable. >> >> But, again, I doubt I understand all the issues here. > > GENERIC requires TREE_ADDRESSABLE to be set on all address-taken > VAR_DECLs, PARM_DECLs and RESULT_DECLs - the gimplifier is the > first to require this for correctness. Setting TREE_ADDRESSABLE > when the address is not taken is harmless and at most results in > missed optimizations (on most entities we are able to clear the > flag later). > > We're currently quite forgiving with this though (still the > gimplifier can generate wrong-code). The trigger of the current > failure removed one "forgiveness", I do plan to remove a few more. > > guojiufu's patch works for me but as said I'm not sure if there's > a better place to set TREE_ADDRESSABLE for entities that have > their address taken - definitely catching the places where > you build an ADDR_EXPR are the most obvious ones. > > Richard. I tested below patch As Ian said, bootstrap pass. ---------------------------------------- diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc index 5d9dbb5d068..529f657598a 100644 --- a/gcc/go/go-gcc.cc +++ b/gcc/go/go-gcc.cc @@ -2943,6 +2943,7 @@ Gcc_backend::implicit_variable(const std::string& name, TREE_STATIC(decl) = 1; TREE_USED(decl) = 1; DECL_ARTIFICIAL(decl) = 1; + TREE_ADDRESSABLE(decl) = 1; if (is_common) { DECL_COMMON(decl) = 1; @@ -3053,6 +3054,7 @@ Gcc_backend::immutable_struct(const std::string& name, TREE_READONLY(decl) = 1; TREE_CONSTANT(decl) = 1; DECL_ARTIFICIAL(decl) = 1; + TREE_ADDRESSABLE(decl) = 1; if (!is_hidden) TREE_PUBLIC(decl) = 1; if (! asm_name.empty()) ---------------------------------------- While, I hacked a patch for OPERATOR_AND. I'm thinking it may be acceptable. BR. Jiufu Guo. ---------------------------------------- Bootstrap failure go [PR100537] In general the Go frontend only takes the addresses of VAR_DECLs or PARM_DECLs. It doesn't bother to set TREE_ADDRESSABLE for global variables for which TREE_STATIC or DECL_EXTERNAL is true. This patch sets TREE_ADDRESSABLE for those symbols which is under OPERATOR_AND (&). Since, I feel, we may able to treat "Unary_expression:OPERATOR_AND" as address_taken operation. Bootstra pass on ppc64le. Is this ok for trunk? gcc/ChangeLog: 2021-05-20 Richard Biener <rguenther@suse.de> Jiufu Guo <guojiufu@linux.ibm.com> PR go/100537 * gimple-expr.c (mark_addressable): Check cfun. gcc/go/ChangeLog: 2021-05-20 Richard Biener <rguenther@suse.de> Jiufu Guo <guojiufu@linux.ibm.com> PR go/100537 * go-gcc.cc (class Gcc_backend): New mark_addresstaken function. (Gcc_backend::mark_addresstaken): New function. * gofrontend/backend.h (class Backend): New mark_addresstaken function. * gofrontend/expressions.cc (Unary_expression::do_get_backend): Call mark_addresstaken. --- gcc/gimple-expr.c | 1 + gcc/go/go-gcc.cc | 13 +++++++++++++ gcc/go/gofrontend/backend.h | 4 ++++ gcc/go/gofrontend/expressions.cc | 1 + 4 files changed, 19 insertions(+) diff --git a/gcc/gimple-expr.c b/gcc/gimple-expr.c index b8c732b632a..f682841391b 100644 --- a/gcc/gimple-expr.c +++ b/gcc/gimple-expr.c @@ -915,6 +915,7 @@ mark_addressable (tree x) if (TREE_CODE (x) == VAR_DECL && !DECL_EXTERNAL (x) && !TREE_STATIC (x) + && cfun != NULL && cfun->gimple_df != NULL && cfun->gimple_df->decls_to_pointers != NULL) { diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc index 5d9dbb5d068..fdbc72a2c75 100644 --- a/gcc/go/go-gcc.cc +++ b/gcc/go/go-gcc.cc @@ -319,6 +319,9 @@ class Gcc_backend : public Backend Bexpression* address_expression(Bexpression*, Location); + void + mark_addresstaken(Bexpression*); + Bexpression* struct_field_expression(Bexpression*, size_t, Location); @@ -1684,6 +1687,16 @@ Gcc_backend::address_expression(Bexpression* bexpr, Location location) return this->make_expression(ret); } +void +Gcc_backend::mark_addresstaken(Bexpression* bexpr) +{ + tree expr = bexpr->get_tree(); + if (expr == error_mark_node) + return; + + mark_addressable(expr); +} + // Return an expression for the field at INDEX in BSTRUCT. Bexpression* diff --git a/gcc/go/gofrontend/backend.h b/gcc/go/gofrontend/backend.h index c5b5d8f2054..45284ed2f0a 100644 --- a/gcc/go/gofrontend/backend.h +++ b/gcc/go/gofrontend/backend.h @@ -315,6 +315,10 @@ class Backend virtual Bexpression* address_expression(Bexpression*, Location) = 0; + // Mark an expression that takes the address of an expression. + virtual void + mark_addresstaken(Bexpression*) = 0; + // Return an expression for the field at INDEX in BSTRUCT. virtual Bexpression* struct_field_expression(Bexpression* bstruct, size_t index, Location) = 0; diff --git a/gcc/go/gofrontend/expressions.cc b/gcc/go/gofrontend/expressions.cc index 5409d269ebf..4e52c8eafba 100644 --- a/gcc/go/gofrontend/expressions.cc +++ b/gcc/go/gofrontend/expressions.cc @@ -5327,6 +5327,7 @@ Unary_expression::do_get_backend(Translate_context* context) } go_assert(!this->create_temp_ || this->expr_->is_multi_eval_safe()); + gogo->backend()->mark_addresstaken(bexpr); ret = gogo->backend()->address_expression(bexpr, loc); break; -- 2.17.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] go/100537 - Bootstrap-O3 and bootstrap-debug fail 2021-05-20 8:59 ` guojiufu @ 2021-05-24 21:46 ` Ian Lance Taylor 0 siblings, 0 replies; 11+ messages in thread From: Ian Lance Taylor @ 2021-05-24 21:46 UTC (permalink / raw) To: guojiufu Cc: Richard Biener, Richard Biener, Ian Lance Taylor, Bill Schmidt, GCC Patches I've committed a patch to the Go frontend that fixes this problem. Thanks for the analysis. Ian On Thu, May 20, 2021 at 1:59 AM guojiufu <guojiufu@linux.ibm.com> wrote: > > On 2021-05-18 14:58, Richard Biener wrote: > > On Mon, 17 May 2021, Ian Lance Taylor wrote: > > > >> On Mon, May 17, 2021 at 1:17 AM Richard Biener via Gcc-patches > >> <gcc-patches@gcc.gnu.org> wrote: > >> > > >> > On Fri, May 14, 2021 at 11:19 AM guojiufu via Gcc-patches > >> > <gcc-patches@gcc.gnu.org> wrote: > >> > > > >> > > On 2021-05-14 15:39, guojiufu via Gcc-patches wrote: > >> > > > On 2021-05-14 15:15, Richard Biener wrote: > >> > > >> On May 14, 2021 4:52:56 AM GMT+02:00, Jiufu Guo > >> > > >> <guojiufu@linux.ibm.com> wrote: > >> > > >>> As discussed in the PR, Richard mentioned the method to > >> > > >>> figure out which VAR was not set TREE_ADDRESSABLE, and > >> > > >>> then cause this failure. It is address_expression which > >> > > >>> build addr_expr (build_fold_addr_expr_loc), but not set > >> > > >>> TREE_ADDRESSABLE. > >> > > >>> > >> > > >>> I drafted this patch with reference the comments from Richard > >> > > >>> in this PR, while I'm not quite sure if more thing need to do. > >> > > >>> So, please have review, thanks! > >> > > >>> > >> > > >>> Bootstrap and regtest pass on ppc64le. Is this ok for trunk? > >> > > >> > >> > > >> I suggest to use mark_addresssable unless we're sure expr is always an > >> > > >> entity where TREE_ADDRESSABLE has the desired meaning. > >> > > > >> > > Thanks, Richard! > >> > > You point out the root concern, I'm not sure ;) > >> > > > >> > > With looking at code "mark_addresssable" and code around > >> > > tree-ssa.c:1013, > >> > > VAR_P, PARM_DECL, and RESULT_DECL are checked before accessing > >> > > TREE_ADDRESSABLE. > >> > > So, just wondering if these entities need to be marked as > >> > > TREE_ADDRESSABLE? > >> > > > >> > > diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc > >> > > index 5d9dbb5d068..85d324a92cc 100644 > >> > > --- a/gcc/go/go-gcc.cc > >> > > +++ b/gcc/go/go-gcc.cc > >> > > @@ -1680,6 +1680,11 @@ Gcc_backend::address_expression(Bexpression* > >> > > bexpr, Location location) > >> > > if (expr == error_mark_node) > >> > > return this->error_expression(); > >> > > > >> > > + if ((VAR_P(expr) > >> > > + || TREE_CODE(expr) == PARM_DECL > >> > > + || TREE_CODE(expr) == RESULT_DECL) > >> > > + TREE_ADDRESSABLE (expr) = 1; > >> > > + > >> > > >> > The root concern is that mark_addressable does > >> > > >> > while (handled_component_p (x)) > >> > x = TREE_OPERAND (x, 0); > >> > > >> > and I do not know the constraints on 'expr' as passed to > >> > Gcc_backend::address_expression. > >> > > >> > I think we need input from Ian here. Most FEs have their own *_mark_addressable > >> > function where they also emit diagnostics (guess this is handled in > >> > the actual Go frontend). > >> > Since Gcc_backend does lowering to GENERIC using a middle-end is probably OK. > >> > >> I doubt I understand all the issues here. > >> > >> In general the Go frontend only takes the addresses of VAR_DECLs or > >> PARM_DECLs. It doesn't bother to set TREE_ADDRESSABLE for global > >> variables for which TREE_STATIC or DECL_EXTERNAL is true. For local > >> variables it sets TREE_ADDRESSABLE based on the is_address_taken > >> parameter to Gcc_backend::local_variable, and similarly for PARM_DECLs > >> and Gcc_backend::parameter_variable. > >> > >> The name in the bug report is for a string initializer, which should > >> be TREE_STATIC == 1 and TREE_PUBLIC == 0. Perhaps the fix is simply > >> to set TREE_ADDRESSABLE in Gcc_backend::immutable_struct and > >> Gcc_backend::implicit_variable. I can't see how it would hurt to set > >> TREE_ADDRESSABLE unnecessarily for a TREE_STATIC variable. > >> > >> But, again, I doubt I understand all the issues here. > > > > GENERIC requires TREE_ADDRESSABLE to be set on all address-taken > > VAR_DECLs, PARM_DECLs and RESULT_DECLs - the gimplifier is the > > first to require this for correctness. Setting TREE_ADDRESSABLE > > when the address is not taken is harmless and at most results in > > missed optimizations (on most entities we are able to clear the > > flag later). > > > > We're currently quite forgiving with this though (still the > > gimplifier can generate wrong-code). The trigger of the current > > failure removed one "forgiveness", I do plan to remove a few more. > > > > guojiufu's patch works for me but as said I'm not sure if there's > > a better place to set TREE_ADDRESSABLE for entities that have > > their address taken - definitely catching the places where > > you build an ADDR_EXPR are the most obvious ones. > > > > Richard. > > I tested below patch As Ian said, bootstrap pass. > > ---------------------------------------- > diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc > index 5d9dbb5d068..529f657598a 100644 > --- a/gcc/go/go-gcc.cc > +++ b/gcc/go/go-gcc.cc > @@ -2943,6 +2943,7 @@ Gcc_backend::implicit_variable(const std::string& > name, > TREE_STATIC(decl) = 1; > TREE_USED(decl) = 1; > DECL_ARTIFICIAL(decl) = 1; > + TREE_ADDRESSABLE(decl) = 1; > if (is_common) > { > DECL_COMMON(decl) = 1; > @@ -3053,6 +3054,7 @@ Gcc_backend::immutable_struct(const std::string& > name, > TREE_READONLY(decl) = 1; > TREE_CONSTANT(decl) = 1; > DECL_ARTIFICIAL(decl) = 1; > + TREE_ADDRESSABLE(decl) = 1; > if (!is_hidden) > TREE_PUBLIC(decl) = 1; > if (! asm_name.empty()) > ---------------------------------------- > While, I hacked a patch for OPERATOR_AND. I'm thinking it may be > acceptable. > > BR. > Jiufu Guo. > ---------------------------------------- > Bootstrap failure go [PR100537] > > In general the Go frontend only takes the addresses of VAR_DECLs or > PARM_DECLs. It doesn't bother to set TREE_ADDRESSABLE for global > variables for which TREE_STATIC or DECL_EXTERNAL is true. > > This patch sets TREE_ADDRESSABLE for those symbols which is under > OPERATOR_AND (&). Since, I feel, we may able to treat > "Unary_expression:OPERATOR_AND" as address_taken operation. > > Bootstra pass on ppc64le. Is this ok for trunk? > > > gcc/ChangeLog: > 2021-05-20 Richard Biener <rguenther@suse.de> > Jiufu Guo <guojiufu@linux.ibm.com> > > PR go/100537 > * gimple-expr.c (mark_addressable): Check cfun. > > gcc/go/ChangeLog: > 2021-05-20 Richard Biener <rguenther@suse.de> > Jiufu Guo <guojiufu@linux.ibm.com> > > PR go/100537 > * go-gcc.cc (class Gcc_backend): New mark_addresstaken function. > (Gcc_backend::mark_addresstaken): New function. > * gofrontend/backend.h (class Backend): New mark_addresstaken function. > * gofrontend/expressions.cc > (Unary_expression::do_get_backend): Call mark_addresstaken. > --- > gcc/gimple-expr.c | 1 + > gcc/go/go-gcc.cc | 13 +++++++++++++ > gcc/go/gofrontend/backend.h | 4 ++++ > gcc/go/gofrontend/expressions.cc | 1 + > 4 files changed, 19 insertions(+) > > diff --git a/gcc/gimple-expr.c b/gcc/gimple-expr.c > index b8c732b632a..f682841391b 100644 > --- a/gcc/gimple-expr.c > +++ b/gcc/gimple-expr.c > @@ -915,6 +915,7 @@ mark_addressable (tree x) > if (TREE_CODE (x) == VAR_DECL > && !DECL_EXTERNAL (x) > && !TREE_STATIC (x) > + && cfun != NULL > && cfun->gimple_df != NULL > && cfun->gimple_df->decls_to_pointers != NULL) > { > diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc > index 5d9dbb5d068..fdbc72a2c75 100644 > --- a/gcc/go/go-gcc.cc > +++ b/gcc/go/go-gcc.cc > @@ -319,6 +319,9 @@ class Gcc_backend : public Backend > Bexpression* > address_expression(Bexpression*, Location); > > + void > + mark_addresstaken(Bexpression*); > + > Bexpression* > struct_field_expression(Bexpression*, size_t, Location); > > @@ -1684,6 +1687,16 @@ Gcc_backend::address_expression(Bexpression* > bexpr, Location location) > return this->make_expression(ret); > } > > +void > +Gcc_backend::mark_addresstaken(Bexpression* bexpr) > +{ > + tree expr = bexpr->get_tree(); > + if (expr == error_mark_node) > + return; > + > + mark_addressable(expr); > +} > + > // Return an expression for the field at INDEX in BSTRUCT. > > Bexpression* > diff --git a/gcc/go/gofrontend/backend.h b/gcc/go/gofrontend/backend.h > index c5b5d8f2054..45284ed2f0a 100644 > --- a/gcc/go/gofrontend/backend.h > +++ b/gcc/go/gofrontend/backend.h > @@ -315,6 +315,10 @@ class Backend > virtual Bexpression* > address_expression(Bexpression*, Location) = 0; > > + // Mark an expression that takes the address of an expression. > + virtual void > + mark_addresstaken(Bexpression*) = 0; > + > // Return an expression for the field at INDEX in BSTRUCT. > virtual Bexpression* > struct_field_expression(Bexpression* bstruct, size_t index, Location) > = 0; > diff --git a/gcc/go/gofrontend/expressions.cc > b/gcc/go/gofrontend/expressions.cc > index 5409d269ebf..4e52c8eafba 100644 > --- a/gcc/go/gofrontend/expressions.cc > +++ b/gcc/go/gofrontend/expressions.cc > @@ -5327,6 +5327,7 @@ > Unary_expression::do_get_backend(Translate_context* context) > } > > go_assert(!this->create_temp_ || > this->expr_->is_multi_eval_safe()); > + gogo->backend()->mark_addresstaken(bexpr); > ret = gogo->backend()->address_expression(bexpr, loc); > break; > > -- > 2.17.1 > > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-05-24 21:46 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-14 2:52 [PATCH] go/100537 - Bootstrap-O3 and bootstrap-debug fail Jiufu Guo 2021-05-14 7:15 ` Richard Biener 2021-05-14 7:39 ` guojiufu 2021-05-14 8:21 ` guojiufu 2021-05-17 8:17 ` Richard Biener 2021-05-18 1:18 ` guojiufu 2021-05-18 1:48 ` Ian Lance Taylor 2021-05-18 6:58 ` Richard Biener 2021-05-18 13:14 ` guojiufu 2021-05-20 8:59 ` guojiufu 2021-05-24 21:46 ` Ian Lance Taylor
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).