stan-dev / stanc3

The Stan transpiler (from Stan to C++ and beyond).
BSD 3-Clause "New" or "Revised" License
140 stars 44 forks source link

[Fix] Properly demote to AoS for variable in for loop when body is an inlined function #1227

Closed WardBrian closed 2 years ago

WardBrian commented 2 years ago

Submission Checklist

Release notes

Fix a bug in the memory pattern optimization where a for loop generated for the function inliner would prevent detection of for loops higher in the control flow graph.

Initially reported by @hsbadr for the rstanarm polr model on the forums here

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)

hsbadr commented 2 years ago

@WardBrian Another one with EpiNow2 R packages when optimizations are enabled:

stanExports_estimate_infections.h(4545): error: identifier "inline_update_Rt_R_sym460__" is undefined
                                            inline_update_Rt_R_sym460__);
                                            ^
WardBrian commented 2 years ago

@hsbadr can you point me toward the Stan code for that model?

hsbadr commented 2 years ago

https://github.com/epiforecasts/EpiNow2/tree/master/inst/stan

SteveBronder commented 2 years ago

@hsbadr did that error happen before this PR?

hsbadr commented 2 years ago

did that error happen before this PR?

I didn't try to build EpiNow2 with stanc3 O1 optimizations before this PR. Should I test it?

hsbadr commented 2 years ago

@SteveBronder It fails with and without this PR if O1 optimizations are enabled. Otherwise, it compiles successfully. So, it's an issue with stanc3 optimizations, not related to this PR.

WardBrian commented 2 years ago

@SteveBronder it's an issue with if the name of the argument before inlining is the same as a variable later in the function.

MRE:

functions {
  vector foo(vector oR) {
    int ot = num_elements(oR);

    vector[ot] R = oR;

    return (R);
  }
}
data {
  int N;
}
transformed parameters {
  vector[N] R;
  vector[N] out;
  out = foo(R);
}
WardBrian commented 2 years ago

This generates code like

 data vector[0] inline_foo_return_sym1__;
  {
    int inline_foo_ot_sym2__;
    inline_foo_ot_sym2__ = num_elements(inline_foo_R_sym3__);
    FnValidateSize__("R", "ot", inline_foo_ot_sym2__);
    vector[inline_foo_ot_sym2__] inline_foo_R_sym3__;
    inline_foo_R_sym3__ = inline_foo_R_sym3__;
    inline_foo_return_sym1__ = inline_foo_R_sym3__;
  }
  out = inline_foo_return_sym1__;

but it should generate code like

data vector[0] inline_foo_return_sym1__;
  {
    int inline_foo_ot_sym2__;
    inline_foo_ot_sym2__ = num_elements(R_renamed);
    FnValidateSize__("R", "ot", inline_foo_ot_sym2__);
    vector[inline_foo_ot_sym2__] inline_foo_R_sym3__;
  }
  out = R_renamed;