Opened 5 years ago

Closed 3 years ago

#5885 closed defect (fixed)

Sliced subscripts in Modelica.ComplexMath.sum fails at code generation

Reported by: Per Östlund Owned by: Mahder Alemseged Gebremedhin
Priority: blocker Milestone: 2.0.0
Component: New Instantiation Version: v1.16.0-dev
Keywords: Cc: Mahder Alemseged Gebremedhin, Adrian Pop, Martin Sjölund

Description (last modified by Per Östlund)

Modelica.ComplexMath.sum is defined as:

function sum 
  input Complex[:] v ;
  output Complex result;
algorithm
  result := Complex(.sum(v[:].re), .sum(v[:].im));
  annotation(Inline = true); 
end sum;

The new frontend flattens this to:

function Modelica.ComplexMath.sum
  input Complex[:] v;
  output Complex result;
algorithm
  result := Complex.'constructor'.fromReal(sum(v[:].re), sum(v[:].im));
end Modelica.ComplexMath.sum;

This causes the following error to occur during code generation:

[CodegenCFunctions.tpl:7515:14-7515:14:writable] Error: Template error: non INDEX(_)
  (i.e., slice) subscripts probably should not reach here.
  Check indexedAssign template..

The question is whether this is something that should be fixed in the frontend or the backend. The subscripts in e.g. v[:].re are superfluous since v.re would mean the same thing, but the same issue occurs with more complicated slice subscripts that can't just be removed. Removing the subscripts also makes the code generation succeed in this case, but the generated code is wrong.

Unfortunately I can't just check what the old frontend does in this case, since it flatten the function to:

function Modelica.ComplexMath.sum
  input Complex[:] v;
  output Complex result;
algorithm
  result := Complex(0.0, 0.0);
end Modelica.ComplexMath.sum;

which is obviously completely wrong.

This issue affects several of the Modelica.Electrical.Batteries models, for example ShowImpedance (output might be outdated, I just fixed the NF so the model passes the frontend).

Change History (7)

comment:1 by Per Östlund, 5 years ago

Description: modified (diff)

comment:2 by Francesco Casella, 4 years ago

Milestone: 1.16.02.0.0
Priority: highblocker

Changed to blocker for 2.0.0, we should support the MSL completely.

comment:3 by Mahder Alemseged Gebremedhin, 4 years ago

The problem is that checks for whether a cref is a scalar are done by this function

https://github.com/OpenModelica/OpenModelica/blob/e293f5ae65e65fc915c09e9c1c133b823c9cbf16/OMCompiler/Compiler/SimCode/SimCodeFunctionUtil.mo#L124

As you can see it only checks for subs in the last part of a cref. Therefore it will treat this as scalar.

I tried fixing this a while back. however, this causes failures in the testsuite. There are things built up on this expectation that only subscripts of the last identifier matter. We have built up on this that it was not easy to fix. Plus at the time working to fix the testsuite (i.e. old NF testing) was deemed a time waste. We might have to revisit it now for NF. Even better that things have changed since then so this might be easy to fix.

I am not sure why removing the subscript gives wrong code. Can you post the generated code?

Last edited 4 years ago by Mahder Alemseged Gebremedhin (previous) (diff)

comment:4 by Mahder Alemseged Gebremedhin, 4 years ago

Owner: changed from Per Östlund to Mahder Alemseged Gebremedhin
Status: newaccepted

Okay I see now that even a fix to crefIsScalar (it does not even check the type anyway) would not solve this exact problem.

Even if we fix that, then you will get another error since we don't really handle 'slices' on the right hand side of assignments. I guess we never run into this so far or we have not really detected slices and just generated them wrong. There is still some work to be done for slices and tuples in codegen.

As for slices on the left hand side of assignments they are treated specifically since we can check at assignment level what the rhs cref is.

We probably need to generate temporary arrays for such uses. Copy each of the specific elements from the record array in to the temp array and use that. This applies whether the 'whole dim' sub is there or not. We need temporaries either way.

comment:5 by Per Östlund, 4 years ago

I actually happened to make some changes (e293f5a) today unrelated to this issue, which now causes the NF to remove the unnecessary : subscripts. But as I mentioned in the description this only causes the ShowImpedance model to fail when compiling the C code instead:

ShowImpedance_total.c:114:164: error: no member named '_re' in 'struct base_array_s'
  data->localData[0]->realVars[1] /* impedance.z.re variable */ = data->simulationInfo->realParameter[254] /* impedance.cellData.R0 PARAM */ + sum_real_array(tmp0._re);
                                                                                                                                                              ~~~~ ^
ShowImpedance_total.c:148:87: error: no member named '_im' in 'struct base_array_s'
  data->localData[0]->realVars[0] /* impedance.z.im variable */ = sum_real_array(tmp7._im);

comment:6 by Mahder Alemseged Gebremedhin, 4 years ago

Ya. Whethe : subscript is there or not we still can not handle them properly. The two just fail in different parts now as the one with no subscript gets a little further being treated and generated as a scalar.

I think we can duplicate the check that we do for lhs crefs to be done for rhs crefs and then generate a temporary array to collect this values. I will see what I can do.

comment:7 by Adrian Pop, 3 years ago

Resolution: fixed
Status: acceptedclosed
Note: See TracTickets for help on using tickets.