Opened 4 years ago

Closed 2 years ago

#5885 closed defect (fixed)

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

Reported by: perost Owned by: mahge930
Priority: blocker Milestone: 2.0.0
Component: New Instantiation Version: v1.16.0-dev
Keywords: Cc: mahge930, adrpo, sjoelund.se

Description (last modified by perost)

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 Changed 4 years ago by perost

  • Description modified (diff)

comment:2 Changed 4 years ago by casella

  • Milestone changed from 1.16.0 to 2.0.0
  • Priority changed from high to blocker

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

comment:3 Changed 4 years ago by mahge930

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 mahge930 (previous) (diff)

comment:4 Changed 4 years ago by mahge930

  • Owner changed from perost to mahge930
  • Status changed from new to accepted

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 Changed 4 years ago by perost

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 Changed 4 years ago by mahge930

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 Changed 2 years ago by adrpo

  • Resolution set to fixed
  • Status changed from accepted to closed
Note: See TracTickets for help on using tickets.