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 )
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 , 5 years ago
Description: | modified (diff) |
---|
comment:2 by , 5 years ago
Milestone: | 1.16.0 → 2.0.0 |
---|---|
Priority: | high → blocker |
comment:3 by , 5 years ago
The problem is that checks for whether a cref is a scalar are done by this function
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?
comment:4 by , 5 years ago
Owner: | changed from | to
---|---|
Status: | new → 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 by , 5 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 , 5 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 , 3 years ago
Resolution: | → fixed |
---|---|
Status: | accepted → closed |
Changed to blocker for 2.0.0, we should support the MSL completely.