Bug 23520 – pragma(inline, false) not applied to nested function declaration

pragma(inline, false) inside a function applies to the outer function, not the nested declaration it is apparently part of. --- void inline(ref int x) { pragma(inline, false) static void noinline(ref int x) { } return noinline(x); } --- Not a single call to the nested static function when compiled with inline.

It seems that the compiler has PragmaDeclaration and PragmaStatement to differentiate between pragmas that are declared at module/aggregate level and pragmas that are declared in function bodies. If you have any kind of pragma that is inside a function body it will then be seen as a pragma statement, irrespective of the fact that it is attached to a single declaration or to a block of statements. I see 2 ways of fixing this: 1. We can implement the expected behavior by attaching the pragma to the declaration, but given the existing machinery in the compiler this will require a lot of rework to transform the existing pragma statement into a pragma declaration and hook in to the existing implementation. 2. Simply deprecate using pragma(inline) statements attached to declarations at statement level. The online pragma that should be allowed at statement level is `pragma(inline, expression);` In the case provided in the bug report you can simply move the pragma(inline) inside the noinline function and it will work. This has the advantage of keeping the implementation simple. I would lean towards the second resolution as it keeps things simple. What do you think Iain?

(In reply to RazvanN from comment #1) > 1. We can implement the expected behavior by attaching the pragma to the > declaration, but given the existing machinery in the compiler this will > require a lot of rework to transform the existing pragma statement into a > pragma declaration and hook in to the existing implementation. Why do you think so? The parser looks like this: --- if (token.value == TOK.semicolon) { nextToken(); _body = null; } else _body = parseStatement(ParseStatementFlags.semi); s = new AST.PragmaStatement(loc, ident, args, _body); break; --- A standalone `pragma(inline);` statement will have no `_body`. This can be checked at semantic with no invasive changes.

(In reply to Iain Buclaw from comment #2) > (In reply to RazvanN from comment #1) > > 1. We can implement the expected behavior by attaching the pragma to the > > declaration, but given the existing machinery in the compiler this will > > require a lot of rework to transform the existing pragma statement into a > > pragma declaration and hook in to the existing implementation. > Why do you think so? > > The parser looks like this: > --- > if (token.value == TOK.semicolon) > { > nextToken(); > _body = null; > } > else > _body = parseStatement(ParseStatementFlags.semi); > s = new AST.PragmaStatement(loc, ident, args, _body); > break; > --- > > A standalone `pragma(inline);` statement will have no `_body`. This can be > checked at semantic with no invasive changes. That is true, but in your original bug report you want to apply the pragma to the declarations inside the body, right? To be able to do that you need to create a PragmaDeclaration and attach it to the the scope of the pragma statement body (struct Scope has a field inlining of type PragmaDeclaration).

> > That is true, but in your original bug report you want to apply the pragma > to the declarations inside the body, right? To be able to do that you need > to create a PragmaDeclaration and attach it to the the scope of the pragma > statement body (struct Scope has a field inlining of type PragmaDeclaration). Of course we can implement a suboptimal algorithm where we go through the members of the body and simply update the inlining field for functions. But then the members will be reiterated for semantic analysis. I guess in the general case this should not be a problem as I do not expect large pragma(inline) blocks inside function bodies.

(In reply to RazvanN from comment #3) > That is true, but in your original bug report you want to apply the pragma > to the declarations inside the body, right? To be able to do that you need > to create a PragmaDeclaration and attach it to the the scope of the pragma > statement body (struct Scope has a field inlining of type PragmaDeclaration). Yes, that can be handled by the statement semantic for PragmaStatement. I think something like the following should suffice: --- Scope* sc2 = sc; scope(exit) if (sc2 != sc) sc2.pop(); // ... if (ps.ident == Id.Pinline) { if (auto fd = sc.func) { if (ps._body) sc2 = new PragmaDeclaration(...).newScope(sc); else fd.inlining = evalPragmaInline(ps.loc, sc, ps.args); } else { ps.error("`pragma(inline)` is not inside a function"); return setError(); } } // ... if (ps._body) { ps._body = ps._body.statementSemantic(sc2); }

Saying that... the way that pragmas are handled in the front-end is all over the shop, with no coherent way to manage them (is a pragma standalone? must have a body? statement only? declaration only? etc...), or allow vendors to extend them (without forking away from upstream dmd) despite this being the intent of pragmas in the spec. See also issue 19109 and issue 19110.

@ibuclaw created dlang/phobos pull request #8642 "std.math.hardware: Fix broken IEEE FP flags tests" mentioning this issue: - std.math.hardware: Fix broken IEEE FP flags tests 1. The `pragma(inline, false)` doesn't do what the author thinks it does (Issue 23520). 2. Change that introduced `blockopt` seems to have a slight misunderstanding about what constant propagation is (`a /= 0.0L` can always be constant propagated regardless of what surrounds it). https://github.com/dlang/phobos/pull/8642

dlang/phobos pull request #8642 "std.math.hardware: Fix broken IEEE FP flags tests" was merged into master: - 36b2f9c6aa3d705c9b2c019ebf4480b5980502ad by Iain Buclaw: std.math.hardware: Fix broken IEEE FP flags tests 1. The `pragma(inline, false)` doesn't do what the author thinks it does (Issue 23520). 2. Change that introduced `blockopt` seems to have a slight misunderstanding about what constant propagation is (`a /= 0.0L` can always be constant propagated regardless of what surrounds it). 3. Undocument tests that depend on the need for forcing operations to disarm the optimizer, and replace with simple tests behind StdDdoc. https://github.com/dlang/phobos/pull/8642

THIS ISSUE HAS BEEN MOVED TO GITHUB https://github.com/dlang/dmd/issues/20195 DO NOT COMMENT HERE ANYMORE, NOBODY WILL SEE IT, THIS ISSUE HAS BEEN MOVED TO GITHUB