Opened 10 years ago

Closed 10 years ago

#3263 closed defect (fixed)

Cpp runtime generates wrong code for MSL matrix function

Reported by: Rüdiger Franke Owned by: Rüdiger Franke
Priority: high Milestone: 1.9.3
Component: Run-time Version: trunk
Keywords: Cc: Marcus Walther, Volker Waurich, Niklas Worschech

Description

The Cpp runtime generates wrong code for the matrix function Modelica.Math.Matrices.sort. The following model:

model SortTest2
  input Real u; // prevent evaluation by frontend
  Real[:, :] M = [3, 4; 1 + u, 2];
  Real[size(M, 1), size(M, 2)] N;
equation
  N = Modelica.Math.Matrices.sort(M);
  annotation(uses(Modelica(version="3.2.1")));
end SortTest2;

results in a strange assignment to the function input M and an unexpanded SMT_ASSIGN_ARR:

OMCppSortTest2Functions.cpp: In member function ‘void Functions::Modelica_Math_Matrices_sort(BaseArray<double>&, bool, bool, Modelica_Math_Matrices_sortRetType&)’:
OMCppSortTest2Functions.cpp:210:47: error: expected primary-expression before ‘.’ token
 sorted_M.setDims(tmp3,tmp4);/*setDims 1*/M =  .targTest91
                                               ^
OMCppSortTest2Functions.cpp:266:10: error: ‘STMT_ASSIGN_ARR’ was not declared in this scope
          STMT_ASSIGN_ARR  RANGE
          ^

Change History (7)

comment:1 by Rüdiger Franke, 10 years ago

r25402 solves the reported compiler errors.

The code for .targTest9<%i%> is not generated anymore if there is no <%outStruct%> before the dot -- don't know if this fixes the root cause; lacking an idea about the intention of targTest.

The implementation of STMT_ASSIGN_ARR RANGE appears straightforward when looking at the already existing STMT_ASSIGN_ARR CALL in the same template algStmtAssignArr.

The compiled model generates wrong results though. There are deficiencies in the code generation for array slices, both rhs and lhs:

  wM2 := sorted_M[j + 1, :];        // generates wM2 := sorted_M[j + 1, NOTHING];
  sorted_M[j + gap + 1, :] := wM2;  // generates sorted_M := wM2
Last edited 10 years ago by Rüdiger Franke (previous) (diff)

comment:2 by Rüdiger Franke, 10 years ago

Owner: changed from somebody to Rüdiger Franke
Status: newassigned

r25403 introduces array slices to solve the lhs assignment problem. Using the new templates algStmtAssignArrCref and daeExpCrefIndexSpec, the above assignment to sorted_M is now translated to:

  vector<Slice> slice;
  slice.push_back(Slice(j + gap + 1));  // index
  slice.push_back(Slice());             // whole dim
  ArraySlice<double>(sorted_M, slice) = wM2;

The SortTest2 example worked after having also fixed the rhs problem in my working copy temporarily!

ArraySlice basically inverts the function create_array_from_shape and it pre-implements the treatment of index sets that is generated explicitly by daeExpCrefRhsIndexSpec.

Ultimately the array slices should also be applicable to rhs, making daeExpCrefRhsIndexSpec and create_array_from_shape obsolete, possibly avoiding some copy operations. But this change needs some more thoughts, because it could break already working things.

comment:3 by Adrian Pop, 10 years ago

rfranke, you forgot to copy the new header file to the build/include directory:
https://test.openmodelica.org/hudson/job/OpenModelica_TEST_CLANG/10720/

/run/shm/OpenModelica_BUILD_CLANG/OpenModelica-clang/build/include/omc/cpp/Core/Modelica.h:198:10: fatal error: 'Core/Math/ArraySlice.h' file not found
#include <Core/Math/ArraySlice.h>

comment:4 by Rüdiger Franke, 10 years ago

should be fixed in r25409

comment:5 by Rüdiger Franke, 10 years ago

r25443 introduces ArraySlice for rhs expressions as well, along with some cleanup in Array.h in r25432, r25436, r25438, r25439, and a new getDim method in r25437.

The template daeExpCrefRhsIndexSpec and the function create_array_from_shape are not used anymore. The former has at least two bugs: it forgets to clear index sets if used in a loop and WHOLEDIM does not work. It should be removed from CodegenCpp.tpl once the new implementation has proven better.

The Modelica statement

  sorted_M[j + 1, :] := sorted_M[j + gap + 1, :];

now is translated to:

  tmp43.clear();
  tmp43.push_back(Slice((1 + (j + gap))));
  tmp43.push_back(Slice());
  ArraySlice<double> tmp43_as(sorted_M, tmp43);
  tmp44.clear();
  tmp44.push_back(Slice((1 + j)));
  tmp44.push_back(Slice());
  ArraySlice<double>(sorted_M, tmp44).assign(tmp43_as);

The generated Cpp code creates no index sets or temporary copies -- its all up to the Array(Slice) implementation. Even though the current version still does a lot internally, it could be optimized without comprising the generated Cpp code.

comment:6 by Rüdiger Franke, 10 years ago

r25567 treats the reported targTest91 problem by not calling the generating template varDefaultValue anymore if already some exp is assigned as default value to an array. The template is still called in the else branch.

comment:7 by Rüdiger Franke, 10 years ago

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