Remove for loop trick for inlining functions w/o multiple returns + optims

[SteveBronder]

Summary

This PR removes for loops surrounding inlined functions if the inlined function does not have more than one return statement while also allow C++ to perform move semantics from the inlined functions inner return to it's outer return object.

After @WardBrian posted #1096 I thought it was odd that inlined functions are always wrapped in a for loop with breaks at the bottom. So this PR looks at a function as we are inlining it and if the function only has one return statement then the inlined function's code is placed into a Block (curly braces) instead of a for loop. For instance the following code is wrapped inside of a for loop that just runs once.

        double inline_sym1__;
        for (int inline_sym3__ = 1; inline_sym3__ <= 1; ++inline_sym3__) {
          inline_sym1__ = (blah(1.0, pstream__) + 0.5);   
          break;
        }

But we can see that there are not multiple returns here and so instead of having a for loop we can just have a block

        double inline_sym1__;
        {
          inline_sym1__ = (blah(1.0, pstream__) + 0.5);   
        }

After cleaning that up, I noticed that the inlining code did a pattern where an variable is declared outside of the block scope, than another variable is declared at the inlined functions scope, and at the end of the inlined functions scope the inner variable is assigned to the outer variable. The below is pseudocode showing what I mean here.

Eigen::Matrix<local_scalar_t__, -1, -1> A_outer;
{
  Eigen::Matrix<local_scalar_t__, -1, -1> A_inner;
  // Do a bunch of stuff with A_inner ...
  assign(A_outer, A_inner);
}

This is the exact kind of place where we want to do move semantics, where at the end of the scope instead of copying A_inner to A_outer we can just hand A_outer the memory of A_inner. This is safe and good because A_inner will be deleted at the end of the inlined functions block scope. To let the C++ know an expression can be moved I made an internal fun_kind called FnMove where if an expression is wrapped in a FnMove the C++ code generator can then call std::move(expr) such as the below.

Eigen::Matrix<local_scalar_t__, -1, -1> A_outer;
{
  Eigen::Matrix<local_scalar_t__, -1, -1> A_inner;
  // Do a bunch of stuff with A_inner ...
  assign(A_outer, std::move(A_inner));
}

While I was reading this PR I was having a hard time following which inline_sym{number}__ was doing what and where. So now when we generate code for inline functions the local inlined variables have the pattern inline_{function}_{orig_name}_sym{number}__. The outer return mentioned above now has the name inline_{func_name}_return_sym{number}__. The iterator for the for loop trick when we have multiple returns now has the pattern inline_{func_name}_{iterator}_sym{number}__.

Submission Checklist

• Run unit tests
• Documentation
  • If a user-facing facing change was made, the documentation PR is here:
  • OR, no user-facing changes were made

Release notes

Replace this text with a short note on what will change if this pull request is merged. This will be included in the release notes.

Copyright and Licensing

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)

[WardBrian]

A few comments questions. The test output on this looks a lot cleaner than the existing stuff!

[SteveBronder]

Also fyi before we merge this I think I should turn it on for all of the tests that get compiled and run it once through Jenkins to help ensure there are no weird edge cases we are missing

[WardBrian]

Do you want this in before the RC? If it can wait we have tons of time to do that

[SteveBronder]

It can absolutely wait. Yeah thinking it over that would be good to do. Though I may need some help as that sort of schema is a little over what I'm used to doing

[WardBrian]

Is there a reason to update Core? We can, but I’ll need to do another windows port and our CI will need updates

[SteveBronder]

No we don't as long as you can tell me how to downgrade the stuff on my laptop after running opam upgrade lol. That's the only reason these are updated

[WardBrian]

You should be able to do opam pin core_kernel=0.14.2

[SteveBronder]

opam pin add core_kernel v0.14.2 did the trick! I think we should add that and opam pin add fmt 0.8.8 to the setup scripts

[WardBrian]

We could, though I think in general there is no reason to run opam upgrade in the switch you use for Stan. Effectively everything is pinned

[WardBrian]

A few style/organization comments. I think that the logic here is sound in general, but I'd appreciate some clarifying comments in the code, especially around the sized 0 sizedtypes created.

[SteveBronder]

Ty for looking this over! lmk if you have more Qs or suggestions

[WardBrian]

How would you feel about doing some git squashing to remove some of the early commits in this PR (things like the opam switch that you reverted)?

Definitely optional but it's a fair number of commits which don't really meaningfully contribute to the history IMO

[WardBrian]

This looks good to me. I propose squashing and merging rather than just a normal merge, but I leave that up to you @SteveBronder

[SteveBronder]

Sounds good! I'll merge on Monday