OpenModelica icon indicating copy to clipboard operation
OpenModelica copied to clipboard

Improve handling of -d=arrayConnect for the new backend

Open casella opened this issue 3 years ago • 32 comments

The -d=arrayConnect implementation currently has issues, most notably:

  • poor performance in some cases #9695
  • generation of additional spurious connection equations as if ports were not connected, see #8014

These issues should eventually be solved, but they are currently hampering the experimentation of the new backend, which activates arrayConnect by default, resulting in 35% of MSL 4.0.0 not passing the NF phase, and only 5% of them passing the new backend.

Since the new backend has a wide range of interesting features beyond its capability to handle connection arrays natively, we should bypass this problem now.

I would suggest the following actions:

  • [ ] when -d=arrayConnect is activated but there are no array connection statements, use the default connection algorithm
  • [ ] when -d=nonfScalarize is activated but -d=arrayConnect is not, any array connection should be scalararized (currently it is skipped or dump in a semi-processed form, which causes the backend to fail)
  • [ ] do not set -d=arrayConnect by default when setting --newBackend

casella avatar Nov 14 '22 10:11 casella

@perost please take care of this at your earliest convenience. @kabdelhak agrees on the approach, and will remove the default setting of -d=arrayConnect when --newBackend is activated, once this fix is in place.

casella avatar Nov 15 '22 10:11 casella

#9733 should fix the first point about only activating the array connection handling if there are arrays, currently with a completely arbitrary minimum array size of 100. We might add a flag later to set the minimum size if needed.

For the second point I turned on scalarization of connections if nfScalarize isn't set or if newBackend is set. The reason for this is to preserve the old behaviour for the existing non-scalarized test cases, because scalarizing the connections doesn't work with the old backend when scalarization is otherwise turned off.

I had some plans for possibly avoiding scalarizing some connections, but this has already taken too much time and making it work with the old backend probably isn't important right now.

perost avatar Nov 22 '22 16:11 perost

I had to add another flag --arrayConnectMinSize for setting the minimum size of array connections for the array connection handling to be used, since we have test cases that don't easily scale up to the fixed minimum size I set earlier.

perost avatar Nov 23 '22 15:11 perost

I had to add another flag --arrayConnectMinSize for setting the minimum size of array connections for the array connection handling to be used, since we have test cases that don't easily scale up to the fixed minimum size I set earlier.

I'm not sure what is the point of this minimum size. We made the arrayConnect algorithm optional because it still has some issues, so we want to be able to skip it. Given one specific case (of generic size N), either it works or it doesn't. If it works, in general I see no reason to de-activate it below a certain size.

In fact, there could be one good reason for that. According to what @ernestokofman told me, this algorithm scales well with the array size, but scales cubically with the number of connect statements (or connection sets, I'm not sure). In this case, it may be reasonable to reserve it for only the largest array connects, using the scalar algorithm for smaller array connects.

@perost, was that what you had in mind?

Before setting an arbitrary value of 100 as default, we should clarify what is the purpose of this flag. I currently don't get it.

casella avatar Nov 24 '22 14:11 casella

@kabdelhak I just started a newInst-newBackend library test job. Let's see what happens with this change.

casella avatar Nov 24 '22 14:11 casella

@casella: Yes, the first point to fix was to only activate the array connection handling if there's actually array connections present. I thought that wasn't a good enough condition since a lot of models have at least some array connections, but they're usually quite small. So I added an additional limit to how large the array connections have to be to activate the array connection handling.

So setting --arrayConnectMinSize to e.g. 10 means that the array connection handling will only activate if there's at least one array connection of size 10 or more (and -d=arrayConnect is also set). The default value is 100. The size of a connection inside a for-loop is also multiplied with the size of the for-loop, since the array connection handling handles scalar connections inside for-loops as arrays.

Of course, this isn't really a good heuristic since a model could have e.g. one array connection but lots of scalar ones, so we might need to improve it later if we want the compiler to automatically choose.

perost avatar Nov 24 '22 14:11 perost

I'm still unsure about this heuristics. I'd say 100 is too large anyway, why would we want to generate 99 separate code chunks to handle 99 connect statements when we can generate a single code chunk that handles all of them? Unless the overhead of arrayConnection algorithm is really huge, I would rather put th limit to 5 or 10 at most.

Besides, you could have small array connects inside a model, but then instantiate a large array of these models. For example, a model has a connect statement on arrays of size 20 (or a for loop doing the same), but then you instantiate an array of 50 such models. With --arrayConnectMeanSize=100 would the NF use the array connection algorithm, or the scalar one?

I expect this pattern to happen quite frequently with large-scale models.

casella avatar Nov 24 '22 16:11 casella

I'm still unsure about this heuristics. I'd say 100 is too large anyway, why would we want to generate 99 separate code chunks to handle 99 connect statements when we can generate a single code chunk that handles all of them? Unless the overhead of arrayConnection algorithm is really huge, I would rather put th limit to 5 or 10 at most.

Besides, you could have small array connects inside a model, but then instantiate a large array of these models. For example, a model has a connect statement on arrays of size 20 (or a for loop doing the same), but then you instantiate an array of 50 such models. With --arrayConnectMeanSize=100 would the NF use the array connection algorithm, or the scalar one?

I expect this pattern to happen quite frequently with large-scale models.

If you have an array of 50 models which each contain a connect statement of size 20, then it would be considered a size of 1000 and use the array connection algorithm with the default limit.

I tried to construct such a model to see what the performance would be but couldn't make one that worked, it seems the array connection algorithm can't handle arrays in arrays somehow. The regular connection algorithm took 50ms, but it would of course produce scalarized equations.

But I think at least for our library testing we probably don't want to use the array connection algorithm pretty much anywhere, since it doesn't really work regardless of array sizes. If @kabdelhak manages to fix it so that it at least works correctly then maybe we can lower the default limit. We might want to add additional restrictions in that case though, since it doesn't handle e.g. stream variables or overconstrained connections.

perost avatar Nov 24 '22 20:11 perost

If you have an array of 50 models which each contain a connect statement of size 20, then it would be considered a size of 1000 and use the array connection algorithm with the default limit.

OK, this sounds reasonable.

I tried to construct such a model to see what the performance would be but couldn't make one that worked, it seems the array connection algorithm can't handle arrays in arrays somehow. The regular connection algorithm took 50ms, but it would of course produce scalarized equations.

You can maybe take some tests from this package TestArray,mo.txt, or just instantiate an array of some of the ScalableTestSuite models that use connects, e.g. TransmissionLineModelica.

But I think at least for our library testing we probably don't want to use the array connection algorithm pretty much anywhere, since it doesn't really work regardless of array sizes. If @kabdelhak manages to fix it so that it at least works correctly then maybe we can lower the default limit. We might want to add additional restrictions in that case though, since it doesn't handle e.g. stream variables or overconstrained connections.

As I understand, 99% of the models of the library testsuite do not have array connects, so I understand in most cases the arrayConnect algorithm would be deactivated anyway.

The only library where we use array connects massively is ScalableTestSuite. In that case I believe it will make sense to make two separate tests: one that always uses arrayConnect (which should eventually work) and one without it, so we can test everything else in the meantime. Regardelss of the array size, so for the former I'd set --arrayConnectMinSize=0

casella avatar Nov 26 '22 18:11 casella

Anyway, I'm still a totally confused about what is going on with the flags.

First of all, in the ScalableTestSuite report using the NF and the old backend, all models pass the frontend successfully, except a few ones that are so large they are probably running out of memory. When the new backend is activated, several models fail, see the report. I ran it on the 24th using 5f522c32bd, after you merged in #9733.

Why does that happen? For example, ScalableTestSuite.Electrical.DistributionSystemAC.ScaledExperiments.DistributionSystemLinear_N_10_M_10 fails with

[OpenModelica/OMCompiler/Compiler/NFFrontEnd/NFSBGraphUtil.mo:194:11-195:71:writable] 
Error: Internal error NFSBGraphUtil.intervalFromExp got unknown expression $i1

The test run has setCommandLineOptions("-d=newInst,-frontEndUnitCheck --newBackend"). I'd say newInst is no longer needed, but shouldn't harm, -frontEndUnitCheck has no effect either, since unit checking is disabled by default, so the only interesting flag here is --newBackend.

Apparently, --newBackend does something which is not nice to NF. One such thing I guess is activating -d=nonfScalarize,arrayConnect, but if I set those flags and get the flat Modelica output, the NF doesn't fail. It gives me wrong connection equations (bogus equations setting the current in each array connection set to zero), but that is another story.

@kabdelhak can you please elaborate on that?

casella avatar Nov 26 '22 18:11 casella

@casella: The error message is from the array connection handling, like I mentioned earlier it seems to have some issues with nested for-loops (the $i1 is most likely an iterator created by the non-scalarized flattening).

perost avatar Nov 26 '22 19:11 perost

A second thing I don't understand in that test report is how the TransmissionLineModelica models fail.

If you check TransmissionLineModelica_N_10, it fails with

Internal error NBackendDAE.lowerEquation failed for
connect(transmissionline.R[i].n, transmissionline.L[i].p)

Since the array connections are definitely below 100 in this model, after #9733 I would expect the NF to flatten those connections to scalar ones. Apparently that doesn't happen for some reason. @perost can you please check?

If instead you check larger models, e.g. TransmissionLineModelica_N_20, the NF is cleared, and the translation fails in the NB with

Error: Internal error NBAdjacency.Matrix.createScalar failed for 
[SCAL] (1) $SEV_0 = 1.0 + transmissionline.R[$i1].alpha * (transmissionline.R[$i1].T_heatPort - transmissionline.R[$i1].T_ref) >= 1e-15 ($RES_EVT_66)

Looks like here maybe the arrayConnect algorithm was activated, so it passed the NF (maybe, I'm not sure.).

But the arrays have size 20, not 100, shouldn't also this model be below the default threshold for --arrayConnectMinSize?

I'm a bit confused 😅

casella avatar Nov 26 '22 19:11 casella

@casella: The error message is from the array connection handling, like I mentioned earlier it seems to have some issues with nested for-loops (the $i1 is most likely an iterator created by the non-scalarized flattening).

OK, so basically all the two-dimensional tests in ScalableTestSuite with overall array connection size >= 100 will fail.

casella avatar Nov 26 '22 19:11 casella

@perost, I guess we have some more basic issues to fix here, sorry for this ticket becoming a bit crowded.

Please consider this MWE:

package TestArrayConnect
  connector Port
    Real v;
    flow Real i;
  end Port;
  
  model M
    Port port;
  equation
    port.v = 10 * port.i;
  end M;
  
  model S
    parameter Integer N = 3;
    M m[N];
  equation
    for i in 1:N-1 loop
      connect(m[i].port, m[i+1].port);
    end for;
  end S;
end TestArrayConnect;

This is the flat scalar model:

class TestArrayConnect.S
  final parameter Integer N = 3;
  Real m[1].port.v;
  Real m[1].port.i;
  Real m[2].port.v;
  Real m[2].port.i;
  Real m[3].port.v;
  Real m[3].port.i;
equation
  m[2].port.v = m[3].port.v;
  m[2].port.v = m[1].port.v;
  m[3].port.i + m[2].port.i + m[1].port.i = 0.0;
  m[1].port.v = 10.0 * m[1].port.i;
  m[2].port.v = 10.0 * m[2].port.i;
  m[3].port.v = 10.0 * m[3].port.i;
end TestArrayConnect.S;

If I flatten it with just -d=nonfScalarize (no arrayConnect) I get:

class TestArrayConnect.S
  final parameter Integer N = 3;
  Real[3] m.port.i;
  Real[3] m.port.v;
equation
  m[2].port.v = m[3].port.v;
  m[2].port.v = m[1].port.v;
  m[2].port.i + m[3].port.i + m[1].port.i = 0.0;
  m.port.i = 0.0;
  for $i1 in 1:3 loop
    m[$i1].port.v = 10.0 * m[$i1].port.i;
  end for;
end TestArrayConnect.S;

The first three equations are the correct flattened scalarized connection equations. Unfortunately the fourth equation is also generated, that basically sets the flow variables of arrays of connectors to zero. This equation should not be generated, otherwise we can't use -nonfScalarize without the array connection algorithm. BTW, it is not even type consistent, since it has an array in the LHS and a scalar in the RHS.

If I flatten it with -d=nonfScalarize,arrayConnect --arrayConnectMinSize=100 I would expect to get the same result, since the array connection algorithm should not be activated on the connection set of size three. However, I get something different:

class TestArrayConnect.S
  final parameter Integer N = 3;
  Real[3] m.port.i;
  Real[3] m.port.v;
equation
  m.port.i = 0.0;
  for $i1 in 1:3 loop
    m[$i1].port.v = 10.0 * m[$i1].port.i;
  end for;
  for i in 1:2 loop
  end for;
end TestArrayConnect.S;

The bogus m.port.i = 0.0 equation is still there; on the other hand, no connection equations are generated at all. The idea was to generate scalar connection equations also in this case 😅

Finally, if I flatten it with -d=nonfScalarize,arrayConnect --arrayConnectMinSize=1, I get

class TestArrayConnect.S
  final parameter Integer N = 3;
  Real[3] m.port.i;
  Real[3] m.port.v;
equation
  for $i1 in 1:3 loop
    m[$i1].port.v = 10.0 * m[$i1].port.i;
  end for;
  m[2].port.v = m[1].port.v;
  m[2].port.i + m[1].port.i = 0.0;
  m[3].port.v = m[2].port.v;
  m[3].port.i = 0.0;
end TestArrayConnect.S;

In this case it seems that the array connection algorithm was activated. Unfortunately, the results are not good either: no array equations were preserved from the array connection equation, and on top of that the flow equations are wrong: one contains two flow variables instead of three, and there is another extra bogus equation m[3].port.i = 0.

I'll try to divide and conquer, one enemy at a time. In this ticket, let's try to make sure (if possible) that when the arrayConnect algorithm is not activated, either because the flag is not set, or because the size of the arrays is below the minimum threshold, then:

  • the array connection equations are scalarized
  • no bogus extra equations on flow variables are generated

so that we get a valid set of equations, even though they are not fully optimized for array-preserving code generation.

This means that with both -d=nonfScalarize and with -d=nonfScalarize --arrayConnectMinSize=100 I expect to get

class TestArrayConnect.S
  final parameter Integer N = 3;
  Real[3] m.port.i;
  Real[3] m.port.v;
equation
  m[2].port.v = m[3].port.v;
  m[2].port.v = m[1].port.v;
  m[2].port.i + m[3].port.i + m[1].port.i = 0.0;
  for $i1 in 1:3 loop
    m[$i1].port.v = 10.0 * m[$i1].port.i;
  end for;
end TestArrayConnect.S;

For the issues with the array connection algorithm, let's continue the discussion on #8014.

casella avatar Nov 26 '22 20:11 casella

@ernestokofman, is your array connect algorithm meant to also work with multi-dimensional arrays?

casella avatar Nov 26 '22 20:11 casella

Yes, our implementation works with multi-dimensional arrays. I don't know about OpenModelica's implementation.

On 26/11/22 17:20, Francesco Casella wrote:

@ernestokofman https://github.com/ernestokofman, is your array connect algorithm meant to also work with multi-dimensional arrays?

— Reply to this email directly, view it on GitHub https://github.com/OpenModelica/OpenModelica/issues/9697#issuecomment-1328107834, or unsubscribe https://github.com/notifications/unsubscribe-auth/A4EJBEJEVLEQHMBVMWAFIKDWKJWIHANCNFSM6AAAAAAR7UETOE. You are receiving this because you were mentioned.Message ID: @.***>

ernestokofman avatar Nov 26 '22 21:11 ernestokofman

Ok, so this is a bit of a mess.

  • If you only use -d=nonfScalarize then you get the old behaviour which doesn't scalarize anything and only works for some cases, which is by design due to the old Bosch test cases relying on this.
  • If you use the --newBackend flag (which implies -d=arrayConnect but apparently not -d=nonfScalarize) then the compiler will select the connection algorithm based on whether there are array connections or not. If the scalarized connection handling is used then the connections will be scalarized regardless of whether nfScalarize is set or not.

However, something I overlooked is that the arrayConnect flag also changes the behaviour of how we deal with for-loops. Normally we just unroll for-equations, but when scalarization is turned off we split each for-equation into one for-equation with the connects and one with non-connects. We then unroll only the for-loops with connects, unless arrayConnect is enabled.

So the issue with turning off arrayConnect if there are no array connections is that we do it too late, and we can't really do it earlier. The solution would be to go through the model and unroll any for-loops that contain connects in that case.

However, I'm starting to wonder if this is perhaps all a bit too overengineered. Do we really need the compiler to automatically choose connection algorithm? It seems to me like the array connection algorithm is really only useful for specific models. So perhaps the best solution is to just change back to having the arrayConnect flag always activate the array connection algorithm, but don't set it by default for the new backend?

perost avatar Nov 28 '22 12:11 perost

Ok, so this is a bit of a mess.

Let's try to sort things out nicely then 😄

  • If you only use -d=nonfScalarize then you get the old behaviour which doesn't scalarize anything and only works for some cases, which is by design due to the old Bosch test cases relying on this.

I was not aware of those test cases. Are they still relevant? Or can we move on with that?

The MARCO compiler and also other developments (e.g. the eFMI flat Modelica output) rely on the fact that you can get correct, non-scalarized flat Modelica out of OMC. "Only work for some cases" is not really an option, it should always produce a correct result, possibly relying on scalarized output if for some reason arrays cannot (yet) be fully preserved. Which seems to be the case with array connections.

* If you use the `--newBackend` flag (which implies `-d=arrayConnect` but apparently not `-d=nonfScalarize`)

Huh? This looks quite weird to me. What is the point of not having array-preserving flat output except for the part that doesn't work?

@kabdelhak can you comment on that?

then the compiler will select the connection algorithm based on whether there are array connections or not.

This behaviour for -d=arrayConnect sounds reasonable to me. No reason to use a potentially buggy array connection algorithm if there are no array connections in the first place.

If the scalarized connection handling is used then the connections will be scalarized regardless of whether nfScalarize is set or not.

Sounds good.

However, something I overlooked is that the arrayConnect flag also changes the behaviour of how we deal with for-loops. Normally we just unroll for-equations, but when scalarization is turned off we split each for-equation into one for-equation with the connects and one with non-connects. We then unroll only the for-loops with connects, unless arrayConnect is enabled.

Sounds reasonable.

So the issue with turning off arrayConnect if there are no array connections is that we do it too late, and we can't really do it earlier. The solution would be to go through the model and unroll any for-loops that contain connects in that case.

I'm not sure I get this. But see below.

However, I'm starting to wonder if this is perhaps all a bit too overengineered.

Could be 😅

Do we really need the compiler to automatically choose connection algorithm? It seems to me like the array connection algorithm is really only useful for specific models.

This is true, but the set of such "specific models" will grow over time. Besides some models in ScalableTestSuite (and also in our HiPerMode benchmarks, that we will publish soon), I expect the mergeComponent feature to become very useful in the future, to generate compact code in models that have many instances of the same class. That will eventually require handling array connects pretty often: if you collect several instances of model M into an array m[100], and M is built by connecting sub-model (which is pretty commonplace), there you go.

So, I would recommend the following setup:

  • -d=nonfScalarize preserves arrays, except for connection equations, which are always scalarized and handled by the old connection algorithm
  • -d=nonfScalarize,arrayConnect preserves arrays and uses the array connection algorithm to handle array connections, regardless of the size. Until the array connection algorithm is reliable enough, scalar connections will still be handled by the old algorithm

What do you think?

So perhaps the best solution is to just change back to having the arrayConnect flag always activate the array connection algorithm,

This is one option, but if I have a model with 100 scalar connections and one array connection, I understand applying the array connect algorithm to the whole thing may cause trouble. I'd prefer to apply it only to the array connection, while using the good old tested and reliable connection algorithm for everything else.

but don't set it by default for the new backend?

Here we need to have a short meeting with @kabdelhak and figure out what is the best setup for him.

casella avatar Nov 29 '22 12:11 casella

@rfranke can perhaps comment on whether the non-scalarized test cases are still relevant for him, and whether he really needs the specific behaviour of the connection handling that they use (which is that it generates array = array equations, but it doesn't work if an array should belong to multiple connection sets).

perost avatar Nov 29 '22 12:11 perost

--newBackend does imply not scalarizing

        // set implied flags to true
        FlagsUtil.enableDebug(Flags.SCODE_INST);
        FlagsUtil.enableDebug(Flags.ARRAY_CONNECT);
        FlagsUtil.disableDebug(Flags.NF_SCALARIZE);

is at the start of the simcode new backend pipeline. Maybe this is not the correct way to do it so Per thought it's not implied. Is there a structure for flags implying other flags?

kabdelhak avatar Nov 29 '22 12:11 kabdelhak

--newBackend does imply not scalarizing

        // set implied flags to true
        FlagsUtil.enableDebug(Flags.SCODE_INST);
        FlagsUtil.enableDebug(Flags.ARRAY_CONNECT);
        FlagsUtil.disableDebug(Flags.NF_SCALARIZE);

is at the start of the simcode new backend pipeline. Maybe this is not the correct way to do it so Per thought it's not implied. Is there a structure for flags implying other flags?

I changed it in #9803 so that we do that at the beginning of the frontend instead. Doing it in SimCode means that they're only set if you use one of the API functions that use translateModel, and not when using omc directly from the command line or calling e.g. instantiateModel.

As part of the change I also removed setting the SCODE_INST flag, since that's the default anyway. If someone wants to use the new backend with the old frontend then that's their problem :sweat_smile:

perost avatar Nov 29 '22 12:11 perost

There is also other behaviour of scalarizing bindings that has changed. I would like to have list/array constructors in expression to stay as they are and not be scalarized so i can extract the iterators and create for-loops from them.

We should really have a meeting to discuss the details!

kabdelhak avatar Nov 30 '22 08:11 kabdelhak

@rfranke can perhaps comment on whether the non-scalarized test cases are still relevant for him, and whether he really needs the specific behaviour of the connection handling that they use (which is that it generates array = array equations, but it doesn't work if an array should belong to multiple connection sets).

BTW I think I was advocating for something similar in #5677. If a certain array connect statement (either involving arrays of ports or with iterators in a for loop) connects two connector arrays that only show up in that specific array connect statement and nowhere else, then the connect equation can be trivially replaced by

  • array equations that make the two corresponding non-flow, non-stream connector variable arrays equal
  • array equations stating that the sum of the pairs of corresponding flow connector variable arrays of the two ports is zeros() with the appropriate size.

Determining which connect statements can undergo this treatment just requires to scan the model once and keep a ledger of how many times a certain port array appears in connect() statements. For all those ones that only show up once, this trivial array connection algorithm can be applied. Easy peasy. It's O(1) w.r.t. the size of the arrays, and O(N) w.r.t. the number of array connect equations in the model.

Couldn't we just do this first step in the NF when -nonfScalarize is activated, before actually involving the array connection algorithm if -d=arrayConnect is set, or flattening array connections to scalar ones if it isn't?

This for sure won't do any harm, and could in fact be hugely beneficial for a number of cases (e.g., the 3D thermal model presented in #5677. It would also make sure that whatever code @rfranke is using would continue to work as usual.

@perost, @kabdelhak, what do you think?

casella avatar Nov 30 '22 22:11 casella

@casella: Dividing the splitting and scalarization of connectors into two separate phases in #9733 was mainly to allow such an approach. The analysis should as you say be fairly easy once the connectors have been split into connector elements.

I didn't consider using it for the array connection algorithm though, just use it to decide on which connectors to scalarize and which to keep as arrays for the normal connection algorithm.

perost avatar Nov 30 '22 23:11 perost

Plan after webmeeting on Dec 2:

  • [X] OK to have divided splitting and scalarization of connectors into two phase in #9733
  • [x] if -d=arrayConnect is set, the array connection algorithm is used throughout - this is good for testing at least for the time being, and easier to implement
  • [x] don't set -d=arrayConnect automatically when setting --newBackend
  • [x] if -d=arrayConnect is not set, the NF should first try to handle simple array connects, then scalarize all remaining array connects, and finally use the default array connection algorithm on the remaining set of originally scalar and scalarized connection sets. This ensures correct flat output in all cases, even though it won't always be the most compact possible result
  • [x] the simple array connect algorithm could then also be used when the NF is expected to fully scalarize the output, because then it will scalarize the corresponding equations (which is trivial), instead of handling many separate connection sets

casella avatar Dec 02 '22 10:12 casella

The NFSBGraphUtil.intervalFromExp got unknown expression error should be fixed in #9838.

The issue was just that the array connection algorithm tried to replace the iterators in the crefs, but the cref traversal functions only traversed the non-scope part of the crefs and missed some iterators. This behaviour was a bit confusing and doesn't seem to save any measurable amount of time anyway, so I just changed it to always traverse the whole cref like one would expect it to.

perost avatar Dec 02 '22 11:12 perost

@perost did you manage tasks 2 and 3 above? I'm eager to see what happens with the new backend when it is passed correct connection equations.

Thanks!

casella avatar Dec 09 '22 15:12 casella

@perost did you manage tasks 2 and 3 above? I'm eager to see what happens with the new backend when it is passed correct connection equations.

Thanks!

No, I just opened #9907 that implements the array-preserving algorithm for single-connected array connectors. So now if two array connectors are only connected to each other we can generate the appropriate array equations instead of scalarizing them. So for e.g. trivial model like:

connector C
  Real e[2];
  flow Real f[2];
end C;

model ArrayConnect5
  C c1[2], c2[2];
equation
  connect(c1, c2);
end ArrayConnect5;

you get:

 class ArrayConnect5
   Real[2, 2] c1.f;
   Real[2, 2] c1.e;
   Real[2, 2] c2.f;
   Real[2, 2] c2.e;
 equation
   c1.e = c2.e;
   for $i1 in 1:2 loop
     for $i2 in 1:2 loop
       (-c1[$i1].f[$i2]) - c2[$i1].f[$i2] = 0.0;
     end for;
   end for;
   for $i1 in 1:2 loop
     for $i2 in 1:2 loop
       c1[$i1].f[$i2] = 0.0;
     end for;
   end for;
   for $i1 in 1:2 loop
     for $i2 in 1:2 loop
       c2[$i1].f[$i2] = 0.0;
     end for;
   end for;
 end ArrayConnect5;

This is only enabled with -d=nonfScalarize, because the old backend doesn't handle the generated for-loops well.

I will take care of the arrayConnect flag in a separate PR, but that shouldn't be much work.

perost avatar Dec 09 '22 15:12 perost

Btw, I disabled one of the existing non-scalarized tests. We didn't handle array connectors when generating flow equations previously and just generated a + b + ... = 0 regardless of type, which somehow worked for those testcases even though it's wrong.

Now we generate the appropriate for-equations instead, but that breaks one of the test cases because SimCodeUtil.createEquation can't handle nested for-equations. I'm not sure if that's an issue for the new backend, but I'm sure someone who knows SimCode better could fix it if needed.

perost avatar Dec 09 '22 15:12 perost

#9908 changes the behaviour of the arrayConnect flag as discussed, i.e. not enabling it by default for the new backend and always using the array connection handling when it's enabled. I also removed the arrayConnectMinSize flag to avoid confusion, since it's no longer used by the compiler.

perost avatar Dec 09 '22 15:12 perost