| Message ID | ri6y2b1hr0p.fsf@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | tree-inline: Fix TREE_READONLY of parameter replacements | expand |
On Wed, 23 Jun 2021, Martin Jambor wrote: > Hi, > > tree-inline leaves behind VAR_DECLs which are TREE_READONLY (because > they are copies of const parameters) but are written to because they > need to be initialized. This patch resets the flag if any > initialization is performed, regardless of if the type needs > construction or not. I wonder if even that is premature optimization - it also removes a still useful comment. So why not at the same place simply do /* Even if P was TREE_READONLY, the new VAR should not be. In the original code, we would have constructed a temporary, and then the function body would have never changed the value of P. However, now, we will be constructing VAR directly. Therefore, it must not be TREE_READONLY. */ TREE_READONLY (var) = 0; ? Since technically when in SSA form you wouldn't need to bother either, so your placement looks both premature and not good enough optimization. Did you check the above suggestion already and it failed for some reason? Thanks, Richard > There are other sources of variables which are incorrectly marked as > TREE_READOLY, but with this patch and a verifier catching them I can > at least compile the Ada run-time library. > > Bootstrapped and tested on x86_64-linux and aarch64-linux, LTO bootstrap > on x86_64-linux is ongoing. OK for trunk if it passes? > > Thanks, > > Martin > > > gcc/ChangeLog: > > 2021-06-22 Martin Jambor <mjambor@suse.cz> > > * tree-inline.c (setup_one_parameter): Set TREE_READONLY of the > param replacement to zero if it is initialized, regardless if it > needs constructing. > --- > gcc/tree-inline.c | 13 +------------ > 1 file changed, 1 insertion(+), 12 deletions(-) > > diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c > index 4a0dc3b6b60..ac8abcd400b 100644 > --- a/gcc/tree-inline.c > +++ b/gcc/tree-inline.c > @@ -3491,18 +3491,6 @@ setup_one_parameter (copy_body_data *id, tree p, tree value, tree fn, > automatically replaced by the VAR_DECL. */ > insert_decl_map (id, p, var); > > - /* Even if P was TREE_READONLY, the new VAR should not be. > - In the original code, we would have constructed a > - temporary, and then the function body would have never > - changed the value of P. However, now, we will be > - constructing VAR directly. The constructor body may > - change its value multiple times as it is being > - constructed. Therefore, it must not be TREE_READONLY; > - the back-end assumes that TREE_READONLY variable is > - assigned to only once. */ > - if (TYPE_NEEDS_CONSTRUCTING (TREE_TYPE (p))) > - TREE_READONLY (var) = 0; > - > tree rhs = value; > if (value > && value != error_mark_node > @@ -3549,6 +3537,7 @@ setup_one_parameter (copy_body_data *id, tree p, tree value, tree fn, > return insert_init_debug_bind (id, bb, var, rhs, NULL); > } > > + TREE_READONLY (var) = 0; > STRIP_USELESS_TYPE_CONVERSION (rhs); > > /* If we are in SSA form properly remap the default definition >
Hi, On Wed, Jun 23 2021, Richard Biener wrote: > On Wed, 23 Jun 2021, Martin Jambor wrote: > >> Hi, >> >> tree-inline leaves behind VAR_DECLs which are TREE_READONLY (because >> they are copies of const parameters) but are written to because they >> need to be initialized. This patch resets the flag if any >> initialization is performed, regardless of if the type needs >> construction or not. > > I wonder if even that is premature optimization - it also removes > a still useful comment. So why not at the same place simply do > > /* Even if P was TREE_READONLY, the new VAR should not be. > In the original code, we would have constructed a > temporary, and then the function body would have never > changed the value of P. However, now, we will be > constructing VAR directly. Therefore, it must not > be TREE_READONLY. */ > TREE_READONLY (var) = 0; > > ? Since technically when in SSA form you wouldn't need to bother > either, so your placement looks both premature and not good enough > optimization. > > Did you check the above suggestion already and it failed for some > reason? > No, I believe the above would work fine. I just looked at the code and the TREE_READONLY resetting is now just above two early exits which only do an insert_init_debug_bind, so I thought it made sense to move the reset of TREE_READONLY only to the same branch which actually generates a write to var. But only because I thought it would be more consistent, and "feel" more logical, not as an optimization. I do not think it makes any difference in practice. So if your feelings are different, I can certainly leave it where it is. As far as the comment is concerned, I like having comments about almost everything, but this one seemed to me a bit too obvious, especially with the TYPE_NEEDS_CONSTRUCTING condition removed. But I can certainly keep it too. Martin
On Wed, 23 Jun 2021, Martin Jambor wrote: > Hi, > > On Wed, Jun 23 2021, Richard Biener wrote: > > On Wed, 23 Jun 2021, Martin Jambor wrote: > > > >> Hi, > >> > >> tree-inline leaves behind VAR_DECLs which are TREE_READONLY (because > >> they are copies of const parameters) but are written to because they > >> need to be initialized. This patch resets the flag if any > >> initialization is performed, regardless of if the type needs > >> construction or not. > > > > I wonder if even that is premature optimization - it also removes > > a still useful comment. So why not at the same place simply do > > > > /* Even if P was TREE_READONLY, the new VAR should not be. > > In the original code, we would have constructed a > > temporary, and then the function body would have never > > changed the value of P. However, now, we will be > > constructing VAR directly. Therefore, it must not > > be TREE_READONLY. */ > > TREE_READONLY (var) = 0; > > > > ? Since technically when in SSA form you wouldn't need to bother > > either, so your placement looks both premature and not good enough > > optimization. > > > > Did you check the above suggestion already and it failed for some > > reason? > > > > No, I believe the above would work fine. I just looked at the code and > the TREE_READONLY resetting is now just above two early exits which only > do an insert_init_debug_bind, so I thought it made sense to move the > reset of TREE_READONLY only to the same branch which actually generates > a write to var. Yeah, but we _do_ create the var and register it as copy of p earlier insert_decl_map (id, p, var); so it seems consistent to adjust things whether or not there are actual writes. > But only because I thought it would be more consistent, and "feel" more > logical, not as an optimization. I do not think it makes any difference > in practice. So if your feelings are different, I can certainly leave > it where it is. > > As far as the comment is concerned, I like having comments about almost > everything, but this one seemed to me a bit too obvious, especially with > the TYPE_NEEDS_CONSTRUCTING condition removed. But I can certainly keep > it too. Yes to both please. OK with those changes. Thanks, Richard.
diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c index 4a0dc3b6b60..ac8abcd400b 100644 --- a/gcc/tree-inline.c +++ b/gcc/tree-inline.c @@ -3491,18 +3491,6 @@ setup_one_parameter (copy_body_data *id, tree p, tree value, tree fn, automatically replaced by the VAR_DECL. */ insert_decl_map (id, p, var); - /* Even if P was TREE_READONLY, the new VAR should not be. - In the original code, we would have constructed a - temporary, and then the function body would have never - changed the value of P. However, now, we will be - constructing VAR directly. The constructor body may - change its value multiple times as it is being - constructed. Therefore, it must not be TREE_READONLY; - the back-end assumes that TREE_READONLY variable is - assigned to only once. */ - if (TYPE_NEEDS_CONSTRUCTING (TREE_TYPE (p))) - TREE_READONLY (var) = 0; - tree rhs = value; if (value && value != error_mark_node @@ -3549,6 +3537,7 @@ setup_one_parameter (copy_body_data *id, tree p, tree value, tree fn, return insert_init_debug_bind (id, bb, var, rhs, NULL); } + TREE_READONLY (var) = 0; STRIP_USELESS_TYPE_CONVERSION (rhs); /* If we are in SSA form properly remap the default definition