stan-dev / stanc3

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

A few more `--O1` issues #1150

Closed rok-cesnovar closed 2 years ago

rok-cesnovar commented 2 years ago

With --O1 there are a few more models that fail to compile to executables. Other than that, it is now looking much better. Apart from these listed below, --O1 compiles with all example models, all "good" integration models in stanc3 and all the tests models I typically look at.

You can see the errors here and here.

@SteveBronder mind taking a look before we do the patch release. Would love to include fixes for these if they are not too involved.

EDIT: updated with a few more actually. Was too quick earlier

WardBrian commented 2 years ago

Are any of these obvious issues in the compiler itself, or are they all C++? I'm happy to help if I can, but if it's C++ stuff it's probably better for @SteveBronder to look at

bob-carpenter commented 2 years ago

Are there any tests that they get the same answers as well as compiling? I think it'd be nice to have the equivalent of unit tests for each of the optimizations.

SteveBronder commented 2 years ago

Just some notes I've found so far on the failures

Bug in fma templating in math library (easy fix):

Possibly an error with optimize_ad_levels overdoing demotion to doubles`

stan/lib/stan_math/stan/math/rev/core/var.hpp:1009:17: error: cannot convert ‘stan::math::var_value<Eigen::Matrix<double, 1, 1, 0, 1, 1>, void>::vari_type* const’ {aka ‘stan::math::vari_value<Eigen::Matrix<double, 1, 1, 0, 1, 1>, void>* const’} to ‘stan::math::var_value<Eigen::Matrix<double, -1, 1> >::vari_type*’ {aka ‘stan::math::vari_value<Eigen::Matrix<double, -1, 1>, void>*’} in assignment

Generating arrays with {} syntax.

I think the easiest thing to do right now would be

  1. Turn off the adlevels optim
  2. Math patch for fma and the Eigen sizing thing (that shouldn't be too hard)

Rok do you mind if I push to #1149 just to see if the adlevel thing is true? That and I can point it to a version of math with the patches

SteveBronder commented 2 years ago

The Generating arrays bug may also be the optimize_ad_levels thing as well

rok-cesnovar commented 2 years ago

@SteveBronder push to that PR at will! Thanks!

WardBrian commented 2 years ago

@bob-carpenter

Are there any tests that they get the same answers as well as compiling? I think it'd be nice to have the equivalent of unit tests for each of the optimizations.

We will be adding these using the reference outputs in https://github.com/stan-dev/performance-tests-cmdstan/tree/master/golds, which we can also expand over time