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

Add promoter expression #1004

Merged
merged 25 commits into from
Jan 12, 2022
Merged

Conversation

SteveBronder
Copy link
Contributor

@SteveBronder SteveBronder commented Oct 15, 2021

@WardBrian I got this up to the point that I think I have all the backend setup. What I'm having trouble figuring out is where to place the actual promotions in the MIR lol!

The thing I'm confused by is where to get the udf signature information. tmk I would need a separate pass where I also include the functions block as I go over each of the other blocks. I was at first thinking about doing this in Stan_math_backend/Transform_mir where I'd have some functions like the below that do go over each block/statement/function/expression adding promotions where we need them.

(*Go over each block updating expressions in functions in statements that need promoted *)
let promote_func_args fun_block prog_block = 
  (*make a table for each udf and it's signature*)
  udf_func_signatures = get_udf_signatures fun_block in 
  (*Go over each statement updating it's functions whose args need promoted *)
  List.map ~f:(promote_func_arg_stmts udf_func_signatures) prog_block

(*iterate over each block in the program doing promotion*)
let do_program_promotion prog = 
   let promote_func_args' = promote_func_args p.functions_block
  Program.map_stmts promote_func_args' p

But should this happen somewhere else sooner? Like should we do it right after we call Ast_to_Mir?

Also do we only need to do this promotion for UDFs or should it also happen for stan math functions. UDFs are a little simpler as there is only ever one signature. Stan math will be a little harder as we would need to choose the "most appropriate" signature

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

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
Copy link
Member

WardBrian commented Oct 15, 2021

I think ideally we would do this during typechecking, if we’re willing to move it into the frontend as well and make it part of the type metadata. Part of #995 is trying to make overloading easier, so we should try to plan ahead if possible

The way I’d do this is change the return types in SignatureMismatch to return something like

type t = Fine | Promotion of int list | SignatureMismatch of error rather than the option type it does now. The int list is just the indecies that need promotions. Note, I’m not staring at the code RN so I have no idea how hard this would be.

does that sound reasonable?

Edit: I realized this is assuming my new typechecker which unifies the use of SignatureMismatch for all function types. I honestly don’t know how to do this in the current typechecker, would need to investigate.

@WardBrian
Copy link
Member

@SteveBronder - if you want to revisit this soon I think it is easier to do in the typechecker now per my above comment. Happy to help if needed

@SteveBronder
Copy link
Contributor Author

Yes! Got lost in pathfinder I'll come back to this in the coming week

@WardBrian WardBrian linked an issue Nov 9, 2021 that may be closed by this pull request
@WardBrian
Copy link
Member

I've linked #373 to this as this is essentially what it was describing, just now applied to the newer types

@WardBrian WardBrian added cpp-codegen feature New feature or request robustness labels Nov 9, 2021
@WardBrian WardBrian force-pushed the feature/add-promoter branch from 116c4ce to e6020bd Compare January 5, 2022 17:06
@WardBrian
Copy link
Member

Looks like I confused Github pretty seriously somehow. I've got this hooked up to the typechecker in an ok-ish way at any rate

@WardBrian WardBrian changed the base branch from complex-promotion to master January 5, 2022 17:08
@WardBrian
Copy link
Member

@SteveBronder this is passing the trivial tests I set up. If you can take another pass at it there's definitely some clean up it could use.

Copy link
Collaborator

@nhuurre nhuurre left a comment

Choose a reason for hiding this comment

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

Ternary operator x ? y : z and array literals {x, y, z} may also generate stan::math::promote_scalar. Would it be feasible to implement those too as a promotion in the MIR?

@@ -93,6 +93,7 @@ let rec eval_expr ?(preserve_stability = false) (e : Expr.Typed.t) =
pattern=
( match e.pattern with
| Var _ | Lit (_, _) -> e.pattern
| Promotion (expr, ut) -> Promotion (eval_expr expr, ut)
Copy link
Collaborator

Choose a reason for hiding this comment

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

[optional] this could be a bit smarter, like promoting literals to literals and collapsing promotion of promotion.

@WardBrian WardBrian requested a review from nhuurre January 11, 2022 19:33
Copy link
Collaborator

@nhuurre nhuurre 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 to me.

@WardBrian
Copy link
Member

Thanks @nhuurre. I’m gonna add some more tests when I fix the merge and update some of the doc. I’ll merge after the tests are done and the doc is approved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants