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.