fix: for loops no longer execute once when condition is already met #1248
Conversation
mhasel
force-pushed
the
for-loops
branch
2 times, most recently
from
9c26969 to
af55a2d
Compare
This PR fixes for loops executing once when the predicate already should not be met for decrementing loops. I have also re-implemented the codegen logic for for-loops, resulting in fewer predecessors and hopefully more readable IR. Resolves #1207
mhasel
changed the title
| FUNCTION main : DINT | ||
| VAR | ||
| i, step, temp, iteration: DINT; | ||
| END_VAR | ||
| step := 1; | ||
| iteration := 0; | ||
|
|
||
| FOR i := 3 TO 10 BY step DO | ||
| step := (step + 1) * -2; | ||
|
|
||
| // i: 3, -1, 5, -9, 17 | ||
| // step: -4, 5, -14, 26 | ||
| iteration := iteration + 1; | ||
| END_FOR | ||
|
|
||
| main := iteration; | ||
| END_FUNCTION |
There was a problem hiding this comment.
I've added two tests with edge cases, were the STEP variable changes signage in the loop body. Do we expect the loop to continue until COUNTER >= END if the loop is determined as incrementing (current behaviour)?
Should we also exit the loop if COUNTER <= START for initially incrementing loops (in which case we'd only expect 1 iteration in the test above)?
Instead of determining if the loop is incrementing/decrementing via the STEP value, should we instead check if START > END and vice versa? i.e.:
incrementing
IF start < end AND counter <= end THEN loop
and decrementing
IF start > end AND counter >= end THEN loop
There was a problem hiding this comment.
Personally I would just mimic what the C language would do in these edge-cases; I suspect it will just check for the condition COUNTER >= END and only stop if it becomes true
There was a problem hiding this comment.
out of interest, what is codesys doing in these cases? is the step by constant? It might be that we always have to invert the check based on the sign.
C has the exit condition defined in the for loop, so it's not the same case.
There was a problem hiding this comment.
yeah, mimicking C is not an ideal solution here unfortunately. I have already tried changing the predicate dynamically based on the sign. This results in most loops executing once, e.g.:
FOR i := 0 TO 10 BY x DO
invert x and check
If x starts out as let's say 2, upon second iteration the check will be IF 2 >= 10. The exception here is if step is larger than the absolute difference between start and end.
I think checking codesys behaviour is the way to go here, too.
Is this a blocker for this story? I don't think this was accounted for previously and might be out of scope for this bug fix. I'd suggest tracking it separately. @ghaith @volsa thoughts?
This PR fixes for loops executing once when the predicate already should not be met for decrementing loops. I have also re-implemented the codegen logic for for-loops, resulting in fewer branches and hopefully more readable IR.
Resolves #1207