Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds mem_pattern needed for SoA and AoS types #944

Merged
merged 8 commits into from
Aug 26, 2021

Conversation

SteveBronder
Copy link
Contributor

This is a subset of #898. There's a lot of stuff going on in that PR so I think it's smarter to do it in smaller pieces.

This PR adds the Common.Helpers.mem_pattern type and uses it in the necessary types that we need for optimization of the tmir and printing the C++ the uses SoA (Struct of Arrays). The largest change is to Stan_math_signatures, where the StanLib types there now have an extra space in their tuple for a mem_pattern.

Everything here is set to AoS (Array of Structs) and so while the mir and tmir have changed, the C++ should not. That said, the C++ from the optimizer has changed slightly, though only the names of the symbol ticks have changed while the actual code has not changed at all.

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

Adds types used for deducing memory patterns

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)

@SteveBronder
Copy link
Contributor Author

@rok-cesnovar would you mind reviewing this?

@rok-cesnovar
Copy link
Member

But of course, would love to. This is exciting.

@SteveBronder
Copy link
Contributor Author

🙏 🙏 🙏 Much appreciated!

Do you happen to know what would cause the error on jenkins with the git clean -xffd?

@rok-cesnovar
Copy link
Member

I tried bumping it but no dice. Paging @serban-nicusor-toptal :)

@serban-nicusor-toptal
Copy link
Contributor

serban-nicusor-toptal commented Aug 17, 2021

Checking

Edit: Fixed now, there was a cache issue on that machine. Sorry for the trouble!

@SteveBronder
Copy link
Contributor Author

Np and thank you!

Copy link
Member

@rok-cesnovar rok-cesnovar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

# Conflicts:
#	test/integration/good/code-gen/transformed_mir.expected
#	test/integration/good/compiler-optimizations/cpp.expected
@rok-cesnovar
Copy link
Member

There seems to be an issue on master with 2 models: https://jenkins.mc-stan.org/blue/organizations/jenkins/stanc3/detail/master/868/pipeline/185

We need to fix those first. Given the error I would say they are related to stan-dev/math#2421 but not super sure.

@SteveBronder
Copy link
Contributor Author

@rok-cesnovar I think your right I just opened a revert on that PR and know what the issue is and can open up a revert revert after the revert happens

@SteveBronder
Copy link
Contributor Author

@rok-cesnovar it looks like cmdstan and Stan hasn't update in the last 5 days so the changes in math haven't propogated upwards. Is there something goofed with the Jenkins stuff?

@rok-cesnovar
Copy link
Member

Math develop has failed and thus not propagated up. Should pass now after stan-dev/math#2564 was merged.

@SteveBronder
Copy link
Contributor Author

I really gotta figure out this precompiled header issue. Maybe we just need to add a %.hpp to the make part that does the .gch in cmdstan so that if any .hpp file changes it needs to rebuild the .gch

@rok-cesnovar
Copy link
Member

Its annoying. I think we arent cleaning up something from previous runs or something along those lines.

@SteveBronder
Copy link
Contributor Author

Yeah I'm looking at this right now in cmdstan and will post up a PR once I get something sorted, but going to merge this for now

@SteveBronder SteveBronder merged commit 766bbf1 into stan-dev:master Aug 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants