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

Improve handling of record bindings #10820

Merged
merged 1 commit into from
Jun 13, 2023
Merged

Conversation

perost
Copy link
Member

@perost perost commented Jun 13, 2023

  • Don't evaluate non-structural parameters.
  • Use tryEvalExp instead of evalExp when trying to evaluate record bindings, to hide any eventual error messages since failure is ok.
  • Improve the function inlining by merging the inlining of record constructors with the inlining of normal function calls, and allowing forced inlining regardless of annotation.

Fixes #10812

@perost perost added the COMP/OMC/Frontend Issue and pull request related to the frontend label Jun 13, 2023
@perost perost self-assigned this Jun 13, 2023
@perost perost enabled auto-merge (squash) June 13, 2023 12:19
@perost perost force-pushed the recordbind branch 2 times, most recently from 4be2d31 to 7911d6e Compare June 13, 2023 13:57
- Don't evaluate non-structural parameters.
- Use `tryEvalExp` instead of `evalExp` when trying to evaluate record
  bindings, to hide any eventual error messages since failure is ok.
- Improve the function inlining by merging the inlining of record
  constructors with the inlining of normal function calls, and allowing
  forced inlining regardless of annotation.

Fixes OpenModelica#10812
@perost perost merged commit 80a64ee into OpenModelica:master Jun 13, 2023
2 checks passed
@mahge
Copy link
Contributor

mahge commented Jun 14, 2023

This has not yet gone through the master regression tests (there is a bit of queue unfortunately), but I think it broke some Buildings models.

You can check the Buildings.Examples.Tutorial.Boiler.System1 model for example. There quite a few more Buildings models that fail the same way.

The issue is around this algorithm

vol.state_start := if size({vol.X_start[1:vol.Medium.nXi]}, 1) == 2 then
 Buildings.Examples.Tutorial.Boiler.System1.vol.Medium.ThermodynamicState(vol.p_start, vol.T_start, {vol.X_start[1]}) else
 Buildings.Examples.Tutorial.Boiler.System1.vol.Medium.ThermodynamicState(vol.p_start, vol.T_start, {vol.X_start[1], 1.0 - vol.X_start[1]});

Specifically, the problem is the {vol.X_start[1:vol.Medium.nXi]} expression which is a matrix.

The following model, which is as close as possible I can make it to the issue without omc optimizing away things, compiles fine

model Test
  Real a[2] = {time + 1, time + 2};
  Real b[1,2];
  Integer i;
algorithm
  i := if time < 1 then 2 else 3;
  b := {a[i-1:i]};
end Test;

Is there anything that you can see that might suggest that the expression {vol.X_start[1:vol.Medium.nXi]} is not getting the correct type?

I thought we were also replacing all slices on the RHS with for-iterator-array expressions. Maybe we are not traversing expressions inside an array ({...}) expression?
Never mind, this is actually not a slice.

perost added a commit to perost/OpenModelica that referenced this pull request Jun 14, 2023
- Temporarily disable inlining of normal functions when splitting record
  bindings, since it causes issues at the moment.
@perost
Copy link
Member Author

perost commented Jun 14, 2023

@mahge: I disabled the part that seems to be causing the issue in #10842. It seems inlining normal functions is causing some issue in this case, it creates the if-expression you mentioned which it then fails to evaluate and moves it to an initial equation because it can't split it.

I don't know what's going wrong, maybe the inlining is incorrect somehow. But that part isn't actually necessary for fixing the issue so I disabled it for now until I have time to take a better look at it.

perost added a commit to perost/OpenModelica that referenced this pull request Jun 14, 2023
- Temporarily disable inlining of normal functions when splitting record
  bindings, since it causes issues at the moment.
@mahge
Copy link
Contributor

mahge commented Jun 14, 2023

Thanks. Hopefully, this makes it to tonight's nightly build and regression test so we can see the effect tomorrow.

The PR build for #10842 was failing due to the wget failure. I have restarted it manually. I will keep an eye on it.

perost added a commit to perost/OpenModelica that referenced this pull request Jun 14, 2023
- Temporarily disable inlining of normal functions when splitting record
  bindings, since it causes issues at the moment.
perost added a commit that referenced this pull request Jun 14, 2023
- Temporarily disable inlining of normal functions when splitting record
  bindings, since it causes issues at the moment.
perost added a commit to perost/OpenModelica that referenced this pull request Jun 15, 2023
- Fix constant evaluation of `size` by evaluating the dimension
  size(s) instead of just returning whatever `Dimension.sizeExp`
  returns, which was causing the regressions from OpenModelica#10820.
- Reactivate inlining of normal functions when trying to split record
  bindings.
@perost
Copy link
Member Author

perost commented Jun 15, 2023

#10844 should fix the issues that occured due to the inlining. The issue was that size({vol.X_start[1:vol.Medium.nXi]}, 1) was constant evaluated to vol.nX, because the evaluation of size didn't actually evaluate the dimension size. With that fixed it now properly evaluates everything and there's no equation being created.

perost added a commit that referenced this pull request Jun 15, 2023
- Fix constant evaluation of `size` by evaluating the dimension
  size(s) instead of just returning whatever `Dimension.sizeExp`
  returns, which was causing the regressions from #10820.
- Reactivate inlining of normal functions when trying to split record
  bindings.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
COMP/OMC/Frontend Issue and pull request related to the frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NF fails in models with fixed = false parameters
2 participants