Opened 4 years ago
Last modified 4 years ago
#6115 new defect
Wrong code generation for *and*
Reported by: | Adrian Pop | Owned by: | Mahder Alemseged Gebremedhin |
---|---|---|---|
Priority: | high | Milestone: | Future |
Component: | Code Generation | Version: | |
Keywords: | Cc: |
Description
The condition in the when:
when (index > 0) and (time >= Array[index]) ... end when
is translated to:
tmp1 = Greater(data->localData[0]->integerVars[1] /* index DISCRETE */,((modelica_integer) 0)); tmp2 = GreaterEq(data->localData[0]->timeValue, (&data->localData[0]->realVars[1] /* Array[1] variable */)[calc_base_index_dims_subs(1, 10, data->localData[0]->integerVars[1] /* index DISCRETE */)]); data->localData[0]->booleanVars[1] /* $whenCondition1 DISCRETE */ = (tmp1 && tmp2);
which will generate an error:
assert | error | Dimension 1 has bounds 1..10, got array subscript 0 assert | info | simulation terminated by an assertion at initialization
The first condition should be checked before the second one!
Change History (8)
comment:1 by , 4 years ago
comment:2 by , 4 years ago
Does Modelica guarantee short circuit evaluation for when conditions?. If not this is technically a modelling problem. Maybe it is better to fix it in the models.
The reason I am saying is because the changes we make to handle this will probably affect all multi-operand Boolean expressions and not just when equations. Even if we can handle this for when equations specifically now what about if conditions?
If we have to handle short circuit evlauation, there are two options I can see
- We can generate the conditions inline:
data->localData[0]->booleanVars[1] = Greater(data->localData[0]->integerVars[1] ... && ...
and let the C compiler do short circuit evaluation.
This will probably not be possible in general due to how Susan works. It is also a bad idea as it will make things unreadable and hard to debug.
OR
- Maybe we can generate them like this.
data->localData[0]->booleanVars[1] /* $whenCondition1 DISCRETE */ = Greater(data->localData[0]->integerVars[1] /* index DISCRETE */,((modelica_integer) 0)); data->localData[0]->booleanVars[1] /* $whenCondition1 DISCRETE */ = data->localData[0]->booleanVars[1] /* $whenCondition1 DISCRETE */ && GreaterEq(data->localData[0]->timeValue, (&data->localData[0]->realVars[1] /* Array[1] variable */)[calc_base_index_dims_subs(1, 10, data->localData[0]->integerVars[1] /* index DISCRETE */)]);
essentially transforming e = a && b && c
into
e = a; e = e && b; e = e && c;
This might be possible if we really have to handle this.
comment:3 by , 4 years ago
I haven't seen anything in the spec on how logical operators should behave, it just says: logical and
. I would handle this as short circuit because it would make evaluation faster if you skip evaluating some part of the condition: when fastCheck() and slowCheck() and slowerCheck()
I would go for this translation:
a = fastCheck(); e = a; if (e) { b = slowCheck(); e = e && b; } if (e) { c = slowerCheck(); e = e && c; }
comment:4 by , 4 years ago
@sjoelund, there is even an example in the spec about this: https://specification.modelica.org/v3.4/Ch3.html#example-guarding-expressions-against-incorrect-evaluation
comment:5 by , 4 years ago
Ok, I read a bit more here:
https://specification.modelica.org/v3.4/Ch3.html#evaluation-order
So the tool *is free to do* the boolean short circuit but is not mandatory.
follow-up: 7 comment:6 by , 4 years ago
Modelica is a declarative language, so I guess a compiler is not only free to do or avoid short circuiting, but also to replace a and b
with b and a
, as they should be equivalent, right?
comment:7 by , 4 years ago
Replying to casella:
a and b
withb and a
, as they should be equivalent, right?
Real x = if a > 0 and log(a) > 0 then a else 0; Real y = if log(a) > 0 and a > 0 then a else 0;
I would accept that x
defined for a =-1 and evaluate y failed
comment:8 by , 4 years ago
Section 3.3 of the specification seems pretty clear to me and would rule out the initially reported code fragment.
One could write
when (index > 0) and (time >=Array[max(1,index)]) ... end when;
to avoid such a problem.
Actually, no. This is Modelica and there is no short-circuit evaluation unless it is guaranteed the expression won't trigger errors. We've had this same or similar appear in tickets multiple times...