On 10/3/19 6:01 AM, Vladislav Ivanishin wrote: > What it does > ============ > > A fundamental limitation of the new approach is that it requires phi > nodes for variables to perform the analysis it needs to issue the > warning for them. No phis - no warning. In particular, it doesn't deal > with register asm variables (see gcc.target/i386/attr-returns_twice-1.c > which I xfailed) because of how they are currently implemented in GCC. > Also, in theory there can be struct members that SRA doesn't scalarize, > but some other compiler might. > > Another "downgrade" from the old implementation is that currently, the > new pass is placed in such a position in the pipeline that it requires > optimize > 0 in order to be run. But there's nothing fundamental about > it; that is something I just haven't experimented with. > > I placed the new pass in tree-ssa-uninit.c, because I used static > functions defined there. The pass ended up using only struct pred_info > and is_pred_expr_subset_of(). I think we document that these warnings are only possible when the optimizer is enabled. So I think the restriction that we have SSA form is reasonable.
> > The main PR tracking the problems of the old implementation is 21161. > I also went through the "See also" list of PRs for it. This new > implementation addresses them all, and the only failing test I saw for > x86_64-pc-linux-gnu is the already mentioned register asm test. Not exactly. There's multiple distinct, but slightly different issues in play here. But I would agree that an implementation of the warning in gimple would be a significant improvement from a design standpoint. > > The existing RTL implementation doesn't check return values of > setjmp/vfork and thus does not distinguish the paths only realizable > upon a non-abnormal [*] return (this is what Wclobbered-pr21161-2.c is > about). The new implementation does deal with it. Correct. I've actually got a patch here which does analysis of return values and their affects on the CFG for RTL for 21161. But it's painful, amazingly painful, because of all the target dependencies you have to make sure to handle. You may want to review the 2018 discussion: https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg185287.html THe 2018 discussion was primarily concerned with fixing the CFG inaccuracies which would in turn fix 61118. But would not fix 21161. Moving the warning into gimple allows us to robustify the implementation. So from design standpoint I absolutely like getting the warning out of RTL and into gimple. Where I have concerns is your implementation leaves the CFG as-is (inaccurate and pessimizing). In fact, I think your implementation relies on the inaccuracy which results in undesirable PHI nodes at the start of the setjmp block (because the long jump incorrectly is modeled as returning to the point just before the setjmp which creates a loop and in turn the PHI nodes you're relying on). I would rather see us attack this as two distinct problems. First would be fixing the CFG. I think the approach Matz sketched out should address the CFG issues and fix 61118 as a side effect. https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg185639.html The key piece of his approach in my mind is moving to a setjmp receiver for each setjmp call and representing the assignment to the setjmp return value in the receiver. Then we'd want to move the warning out of RTL and into gimple. I fear that we may have to rethink some of your current approach because if the CFG is fixed, you won't have the PHI nodes at the start of the block with the setjmp call. You might instead be able to look at the PHI nodes of the successor block or perhaps wire it into the out-of-ssa code which does life analysis and analyze the objects that are live across the call point. Jeff
