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 Martin Sjölund, 4 years ago

The first condition should be checked before the second one!

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...

comment:2 by Mahder Alemseged Gebremedhin, 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 Adrian Pop, 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:5 by Adrian Pop, 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.

comment:6 by Francesco Casella, 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?

in reply to:  6 comment:7 by anonymous, 4 years ago

Replying to casella:

a and b with b 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 Francesco Casella, 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.

Note: See TracTickets for help on using tickets.