for loop in algorithm section gets wrong values

[r8KZbITHbJBHykci96Mg@…]

The following example is a simple demonstration of the bug.

connector AgentInPort
    input Real Temp;
    input Real MassFlow;
end AgentInPort;

connector AgentOutPort
    output Real Temp;
    output Real MassFlow;
end AgentOutPort;


model MyAgentAcc
    parameter Integer nports;
    AgentInPort[nports] In;
    AgentOutPort Out;
    Real sumTemp, sumMassFlow;
    Real mf[nports], te[nports];
algorithm

    for i in 1:nports loop
        te[i] := In[i].Temp;
        sumTemp := sumTemp + In[i].Temp;
        mf[i] := In[i].MassFlow;
        sumMassFlow := sumMassFlow + In[i].MassFlow;
    end for;
equation
    Out.MassFlow = sumMassFlow;
    Out.Temp = sumTemp;
end MyAgentAcc;


model Tester
    MyAgentAcc TestAcc(nports=2);
    AgentOutPort VL1, VL2;
equation
    VL1.Temp = 14;
    VL1.MassFlow = 1;
    VL2.Temp = 28;
    VL2.MassFlow = 2;
    connect(VL1, TestAcc.In[1]);
    connect(VL2, TestAcc.In[2]);
end Tester;

The expected output is:
TestAcc.Out.MassFlow = 3
TestAcc.Out.Temp = 42
TestAcc.mf[1] = 1
TestAcc.mf[2] = 2
TestAcc.te[1] = 14
TestAcc.te[2] = 28

But the output is:
TestAcc.Out.MassFlow = 15
TestAcc.Out.Temp = 16
TestAcc.mf[1] = 1
TestAcc.mf[2] = 14
TestAcc.te[1] = 14
TestAcc.te[2] = 2

In the for loop i=1 is working as expected, but with i=2 we get wrong values!

[Francesco Casella]

I had a quick look at this. We have these alias variables

1: TestAcc.In[2].MassFlow:VARIABLE(flow=false )  = VL2.MassFlow P.Tester, P.MyAgentAcc, P.AgentInPort type: Real [2] 
2: TestAcc.In[2].Temp:VARIABLE(flow=false )  = VL2.Temp P.Tester, P.MyAgentAcc, P.AgentInPort type: Real [2] 
3: TestAcc.In[1].MassFlow:VARIABLE(flow=false )  = VL1.MassFlow P.Tester, P.MyAgentAcc, P.AgentInPort type: Real [2] 
4: TestAcc.In[1].Temp:VARIABLE(flow=false )  = VL1.Temp P.Tester, P.MyAgentAcc, P.AgentInPort type: Real [2] 
5: TestAcc.Out.Temp:VARIABLE(flow=false )  = TestAcc.sumTemp P.Tester, P.MyAgentAcc, P.AgentOutPort type: Real 
6: TestAcc.Out.MassFlow:VARIABLE(flow=false )  = TestAcc.sumMassFlow P.Tester, P.MyAgentAcc, P.AgentOutPort type: Real 

and then the generated C-code looks like this:

    modelica_integer i;
    for(i = ((modelica_integer) 1); in_range_integer(i, tmp0, tmp2); i += tmp1)
    {
      (&data->localData[0]->realVars[4] /* TestAcc.te[1] variable */)[calc_base_index_dims_subs(1, 2, i)] = (&data->localData[0]->realVars[7] /* VL1.Temp variable */)[calc_base_index_dims_subs(1, 2, i)];
      data->localData[0]->realVars[3] /* TestAcc.sumTemp variable */ = data->localData[0]->realVars[3] /* TestAcc.sumTemp variable */ + (&data->localData[0]->realVars[7] /* VL1.Temp variable */)[calc_base_index_dims_subs(1, 2, i)];
      (&data->localData[0]->realVars[0] /* TestAcc.mf[1] variable */)[calc_base_index_dims_subs(1, 2, i)] = (&data->localData[0]->realVars[6] /* VL1.MassFlow variable */)[calc_base_index_dims_subs(1, 2, i)];
      data->localData[0]->realVars[2] /* TestAcc.sumMassFlow variable */ = data->localData[0]->realVars[2] /* TestAcc.sumMassFlow variable */ + (&data->localData[0]->realVars[6] /* VL1.MassFlow variable */)[calc_base_index_dims_subs(1, 2, i)];

it looks like in the algorithm, VL1.Temp, which is an alias of TestAcc.In[1].Temp was chosen instead of TestAcc.In[i].Temp

@mahge930, would you mind having a look?

Thanks!

[Mahder Alemseged Gebremedhin]

I guess this is the classic removeSimpleEquations alias issue. It alias eliminates array variables. In this case it has eliminated TestAcc.In[1].Temp and replaces all its occurrences with VL1.Temp. I think you read the info the other way around.

When iterating over an array in a simulation code we use the first element as reference for offsetting. In this case that is TestAcc.In[1].Temp. It works for index 1 since it is mapped to VL1.Temp anyway. On index 2 we try to jump/offset one unit from TestAcc.In[1].Temp which was supposed to take us to TestAcc.In[2].Temp. This works because we sort the variables lexicographically to give them an entry in the big simulation array which contains all variables of the same type.

Here is the current ordering:

index	variable	aliased vars
0	TestAcc.mf[1]
1	TestAcc.mf[2]
2	TestAcc.sumMassFlow	TestAcc.Out.MassFlow
3	TestAcc.sumTemp	TestAcc.Out.Temp
4	TestAcc.te[1]
5	TestAcc.te[2]
6	VL1.MassFlow	TestAcc.In[1].MassFlow
7	VL1.Temp	TestAcc.In[1].Temp
8	VL2.MassFlow	TestAcc.In[2].MassFlow
9	VL2.Temp	TestAcc.In[2].Temp

As you can see if we try to offset 1 unit from TestAcc.In[1].Temp, we offest 1 unit from VL1.Temp and end up getting VL2.MassFlow.

This elimination of array variables usually works just fine. But in cases like this where we want to iterate over the array or we want to send the array to a function, it causes these problems. The worst part is that we get no compilation error as you saw.

I think I remember trying to fix this before by modifying removeSimpleEquations. However it lead to so many failures in other models that I gave up.

I will take a look again but I think this is a difficult issue to fix because it's not really about this model. Instead it is about the other models that fail for 100 different reasons when you disable this.

[Mahder Alemseged Gebremedhin]

You can try simulating the model with the flags

--preOptModules-=removeSimpleEquations --postOptModules-=removeSimpleEquations

to disable the removeSimpleEquations module for now.

[Francesco Casella]

I understand from our discussion during the devmeeting that the basic problem is that the code generation expects the backend to do certain things in order to work properly. In this case, I understand it expects the first element of an array not to be replaced by an alias. I'm not sure if this can be really classified as an issue with removeSimpleEquation.

In any case, would it be possible to implement a simple rule for alias elimination, i.e. to always prefer array elements to scalar when selecting the representative variable?

[Mahder Alemseged Gebremedhin]

Replying to casella:

I understand from our discussion during the devmeeting that the basic problem is that the code generation expects the backend to do certain things in order to work properly. In this case, I understand it expects the first element of an array not to be replaced by an alias.

Yes. But not just the first element. If any element of an array is alias eliminated ( but not the whole array) we might have some issues.

I'm not sure if this can be really classified as an issue with removeSimpleEquation.

Well yes and no. Of course, we can unroll algorithm loops such as the one in this example like we do for for equations (assuming the loop range is bound by structural parameter). Then every index will be known beforehand and removeSimpleEquations can replace all occurrences of the array member. However, we still will have problems when we want to use the whole array, e.g. sending it to a function. In that case we will have to first check if any of its members is alias eliminated. If so we will need to construct a whole new array update the eliminated variable with its alias and then send it to a function. Although this does not often happen we will have to do the check at codegen time for every array we pass to a function.

Both approaches might work.

In any case, would it be possible to implement a simple rule for alias elimination, i.e. to always prefer array elements to scalar when selecting the representative variable?

Yes, the simplest (and maybe correct?) approach should be to fix removeSimpleEquations to prefer non array variables over array variables whenever it can. This should be, in theory, possible for the large majority of models we deal with.

[Francesco Casella]

@mahge930, you convinced me 100% that doing alias elimination of parts of arrays is not a good idea.

I opened #5868 specifically on this topic

[Francesco Casella]

Retargeted to 1.17.0 after 1.16.0 release

[Francesco Casella]

Retargeted to 1.18.0 because of 1.17.0 timed release.

[Francesco Casella]

Ticket retargeted after milestone closed