From ba3d4f88701dd64e19df0bb6e909c2e3d602b40a Mon Sep 17 00:00:00 2001 From: Chris Campbell Date: Tue, 26 Nov 2024 08:00:14 -0800 Subject: [PATCH 1/5] fix: cast index values to double when used in expression position in C code generation --- packages/compile/src/generate/gen-expr.js | 27 +++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/packages/compile/src/generate/gen-expr.js b/packages/compile/src/generate/gen-expr.js index 02aea503..cbee55fe 100644 --- a/packages/compile/src/generate/gen-expr.js +++ b/packages/compile/src/generate/gen-expr.js @@ -69,14 +69,14 @@ export function generateExpr(expr, ctx) { // plus one (since Vensim indices are one-based). const dimId = expr.varId const indexCode = ctx.cVarIndex(dimId) - return `(${indexCode} + 1)` + return `(${indexExpr(indexCode, ctx)} + 1)` } else if (isIndex(expr.varId)) { // This is a reference to a subscript/index that is being used in expression position. // In place of the subscript, emit the numeric index value of the subscript plus one // (since Vensim indices are one-based). const subId = expr.varId const indexValue = sub(subId).value - return `${indexValue + 1}` + return indexExpr(`${indexValue + 1}`, ctx) } else { throw new Error(`Unresolved variable reference '${expr.varName}' in code gen for '${ctx.variable.modelLHS}'`) } @@ -1006,3 +1006,26 @@ function minFunc(ctx) { throw new Error(`Unhandled output format '${ctx.outFormat}'`) } } + +/** + * Return the C or JS code for a subscript or dimension (loop index variable) used in + * expression position. + * + * @param {string} indexValue The index number or code. + * @param {GenExprContext} ctx The context used when generating code for the expression. + * @return {string} The generated C/JS code. + */ +function indexExpr(indexValue, ctx) { + switch (ctx.outFormat) { + case 'c': + // In the C case, we need to cast to double since the index variable will be + // of type `size_t`, which is an unsigned type, but we want a signed type for + // the rare cases where math is involved that makes it go negative + return `((double)${indexValue})` + case 'js': + // In the JS case, no cast is necessary + return indexValue + default: + throw new Error(`Unhandled output format '${ctx.outFormat}'`) + } +} From fa9b85b2882c3e1944a7c5b058e74fbb74566871 Mon Sep 17 00:00:00 2001 From: Chris Campbell Date: Fri, 13 Dec 2024 15:13:10 -0800 Subject: [PATCH 2/5] fix: only cast to double if index is a variable reference --- packages/compile/src/generate/gen-expr.js | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/compile/src/generate/gen-expr.js b/packages/compile/src/generate/gen-expr.js index ee994986..d05f5b98 100644 --- a/packages/compile/src/generate/gen-expr.js +++ b/packages/compile/src/generate/gen-expr.js @@ -74,7 +74,7 @@ export function generateExpr(expr, ctx) { // (since Vensim indices are one-based). const subId = expr.varId const indexValue = sub(subId).value - return indexExpr(`${indexValue + 1}`, ctx) + return `${indexValue + 1}` } else { throw new Error(`Unresolved variable reference '${expr.varName}' in code gen for '${ctx.variable.modelLHS}'`) } @@ -1035,7 +1035,13 @@ function indexExpr(indexValue, ctx) { // In the C case, we need to cast to double since the index variable will be // of type `size_t`, which is an unsigned type, but we want a signed type for // the rare cases where math is involved that makes it go negative - return `((double)${indexValue})` + if (isNaN(indexValue)) { + // This is a (non-numeric) loop index variable reference, so cast to double + return `((double)${indexValue})` + } else { + // This is a numeric index, no cast is necessary + return indexValue + } case 'js': // In the JS case, no cast is necessary return indexValue From 9e3e560e38782831c1f4402ca96c991470a74096 Mon Sep 17 00:00:00 2001 From: Chris Campbell Date: Fri, 13 Dec 2024 15:13:56 -0800 Subject: [PATCH 3/5] test: update tests to expect cast --- .../src/generate/gen-equation-c.spec.ts | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/packages/compile/src/generate/gen-equation-c.spec.ts b/packages/compile/src/generate/gen-equation-c.spec.ts index 39b4e956..6c69016b 100644 --- a/packages/compile/src/generate/gen-equation-c.spec.ts +++ b/packages/compile/src/generate/gen-equation-c.spec.ts @@ -291,7 +291,7 @@ describe('generateEquation (Vensim -> C)', () => { expect(genC(vars.get('_x'))).toEqual(['_x = 1.0;']) expect(genC(vars.get('_y'))).toEqual([ 'for (size_t i = 0; i < 2; i++) {', - '_y[i] = _IF_THEN_ELSE((i + 1) == _x, 1.0, 0.0);', + '_y[i] = _IF_THEN_ELSE((((double)i) + 1) == _x, 1.0, 0.0);', '}' ]) }) @@ -304,7 +304,7 @@ describe('generateEquation (Vensim -> C)', () => { expect(vars.size).toBe(1) expect(genC(vars.get('_y'))).toEqual([ 'for (size_t i = 0; i < 2; i++) {', - '_y[i] = _IF_THEN_ELSE((i + 1) == 2, 1.0, 0.0);', + '_y[i] = _IF_THEN_ELSE((((double)i) + 1) == 2, 1.0, 0.0);', '}' ]) }) @@ -650,7 +650,11 @@ describe('generateEquation (Vensim -> C)', () => { x[DimA] = DimB ~~| `) expect(vars.size).toBe(1) - expect(genC(vars.get('_x'))).toEqual(['for (size_t i = 0; i < 2; i++) {', '_x[i] = (__map_dimb_dima[i] + 1);', '}']) + expect(genC(vars.get('_x'))).toEqual([ + 'for (size_t i = 0; i < 2; i++) {', + '_x[i] = (((double)__map_dimb_dima[i]) + 1);', + '}' + ]) }) it('should work for 1D equation with one mapped dimension name used in subscript position (separated/non-apply-to-all)', () => { @@ -680,7 +684,7 @@ describe('generateEquation (Vensim -> C)', () => { expect(genC(vars.get('_selected_a_index'), 'init-constants')).toEqual(['_selected_a_index = 1.0;']) expect(genC(vars.get('_x'))).toEqual([ 'for (size_t i = 0; i < 2; i++) {', - '_x[i] = _IF_THEN_ELSE((i + 1) == _selected_a_index, 1.0, 0.0);', + '_x[i] = _IF_THEN_ELSE((((double)i) + 1) == _selected_a_index, 1.0, 0.0);', '}' ]) }) @@ -708,7 +712,7 @@ describe('generateEquation (Vensim -> C)', () => { expect(genC(vars.get('_x'))).toEqual([ 'for (size_t i = 0; i < 2; i++) {', 'for (size_t j = 0; j < 2; j++) {', - '_x[i][j] = ((i + 1) * 10.0) + (j + 1);', + '_x[i][j] = ((((double)i) + 1) * 10.0) + (((double)j) + 1);', '}', '}' ]) @@ -738,7 +742,7 @@ describe('generateEquation (Vensim -> C)', () => { expect(genC(vars.get('_x'))).toEqual([ 'for (size_t i = 0; i < 2; i++) {', 'for (size_t j = 0; j < 2; j++) {', - '_x[i][j] = ((i + 1) * 10.0) + (j + 1);', + '_x[i][j] = ((((double)i) + 1) * 10.0) + (((double)j) + 1);', '}', '}' ]) @@ -3224,7 +3228,7 @@ describe('generateEquation (Vensim -> C)', () => { expect(genC(vars.get('_x[_five]'), 'init-constants')).toEqual(['_x[4] = 5.0;']) expect(genC(vars.get('_y'))).toEqual([ 'for (size_t i = 0; i < 3; i++) {', - '_y[i] = _x[_dimx[(size_t)(2 + ((i + 1) - 1.0))]];', + '_y[i] = _x[_dimx[(size_t)(2 + ((((double)i) + 1) - 1.0))]];', '}' ]) }) From 1dc0458a08e94898e54fa081247f51eed3b5f1bc Mon Sep 17 00:00:00 2001 From: Chris Campbell Date: Fri, 13 Dec 2024 15:23:15 -0800 Subject: [PATCH 4/5] test: update subscript sample model to include case where subscript math can go negative --- models/subscript/subscript.dat | 27 +++++++++++++++++++++++++++ models/subscript/subscript.mdl | 8 ++++++++ 2 files changed, 35 insertions(+) diff --git a/models/subscript/subscript.dat b/models/subscript/subscript.dat index 6e37f009..3952cc77 100644 --- a/models/subscript/subscript.dat +++ b/models/subscript/subscript.dat @@ -185,3 +185,30 @@ v[A2] v[A3] 0 0 1 0 +w[X1,Y1] +0 0 +1 0 +w[X1,Y2] +0 -1 +1 -1 +w[X1,Y3] +0 -2 +1 -2 +w[X2,Y1] +0 1 +1 1 +w[X2,Y2] +0 0 +1 0 +w[X2,Y3] +0 -1 +1 -1 +w[X3,Y1] +0 2 +1 2 +w[X3,Y2] +0 1 +1 1 +w[X3,Y3] +0 0 +1 0 diff --git a/models/subscript/subscript.mdl b/models/subscript/subscript.mdl index 17ff1f75..5124d15d 100755 --- a/models/subscript/subscript.mdl +++ b/models/subscript/subscript.mdl @@ -59,6 +59,14 @@ v[DimA] = IF THEN ELSE (DimA = A2, 1, 0) ~:SUPPLEMENTARY | +DimX: (X1-X3) ~~| +DimY: (Y1-Y3) ~~| +w[DimX,DimY] = DimX - DimY + ~ + ~ dimensions used in expression position (where expression result can go negative) + ~:SUPPLEMENTARY + | + ******************************************************** .Control ********************************************************~ From d48e4859b7a686c3d3d9f423d2d39e15379affdf Mon Sep 17 00:00:00 2001 From: Chris Campbell Date: Fri, 13 Dec 2024 15:33:20 -0800 Subject: [PATCH 5/5] test: update unit tests that exercise the updated "subscript" sample model --- packages/compile/src/model/read-equations.spec.ts | 4 ++++ packages/compile/src/model/read-subscripts.spec.ts | 14 ++++++++++++-- packages/compile/src/model/read-variables.spec.ts | 3 +++ 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/packages/compile/src/model/read-equations.spec.ts b/packages/compile/src/model/read-equations.spec.ts index d3a6c2ac..dfae1f52 100644 --- a/packages/compile/src/model/read-equations.spec.ts +++ b/packages/compile/src/model/read-equations.spec.ts @@ -10174,6 +10174,10 @@ describe('readEquations', () => { refId: '_v', subscripts: ['_dima'] }), + v('w[DimX,DimY]', 'DimX-DimY', { + refId: '_w', + subscripts: ['_dimx', '_dimy'] + }), v('FINAL TIME', '1', { refId: '_final_time', varType: 'const' diff --git a/packages/compile/src/model/read-subscripts.spec.ts b/packages/compile/src/model/read-subscripts.spec.ts index 760a8b1a..034c82bf 100644 --- a/packages/compile/src/model/read-subscripts.spec.ts +++ b/packages/compile/src/model/read-subscripts.spec.ts @@ -607,7 +607,9 @@ describe('readSubscriptRanges + resolveSubscriptRanges', () => { dim('DimB', ['B1', 'B2', 'B3'], undefined, undefined, [dimMapping('DimA')], { _dima: [] }), - dim('DimC', ['C1', 'C2', 'C3', 'C4', 'C5']) + dim('DimC', ['C1', 'C2', 'C3', 'C4', 'C5']), + dim('DimX', ['X1', 'X2', 'X3']), + dim('DimY', ['Y1', 'Y2', 'Y3']) ]) const resolvedSubs = readAndResolveSubscripts('subscript') @@ -641,6 +643,8 @@ describe('readSubscriptRanges + resolveSubscriptRanges', () => { _dima: ['_b1', '_b2', '_b3'] }), dim('DimC', ['C1', 'C2', 'C3', 'C4', 'C5']), + dim('DimX', ['X1', 'X2', 'X3']), + dim('DimY', ['Y1', 'Y2', 'Y3']), // { name: '_a1', value: 0, size: 1, family: '_dima', mappings: {} }, sub('A1', 'DimA', 0), // { name: '_a2', value: 1, size: 1, family: '_dima', mappings: {} }, @@ -657,7 +661,13 @@ describe('readSubscriptRanges + resolveSubscriptRanges', () => { sub('C2', 'DimC', 1), sub('C3', 'DimC', 2), sub('C4', 'DimC', 3), - sub('C5', 'DimC', 4) + sub('C5', 'DimC', 4), + sub('X1', 'DimX', 0), + sub('X2', 'DimX', 1), + sub('X3', 'DimX', 2), + sub('Y1', 'DimY', 0), + sub('Y2', 'DimY', 1), + sub('Y3', 'DimY', 2) ]) }) }) diff --git a/packages/compile/src/model/read-variables.spec.ts b/packages/compile/src/model/read-variables.spec.ts index eebd965b..35a56310 100644 --- a/packages/compile/src/model/read-variables.spec.ts +++ b/packages/compile/src/model/read-variables.spec.ts @@ -1933,6 +1933,9 @@ describe('readVariables', () => { v('v[DimA]', 'IF THEN ELSE(DimA=A2,1,0)', { subscripts: ['_dima'] }), + v('w[DimX,DimY]', 'DimX-DimY', { + subscripts: ['_dimx', '_dimy'] + }), v('FINAL TIME', '1'), v('INITIAL TIME', '0'), v('SAVEPER', 'TIME STEP'),