Skip to content

Commit

Permalink
fix incorrect removal of self-assignments (#1357)
Browse files Browse the repository at this point in the history
## Summary

Fix a bug with the MIR-based alias analysis that caused assignments
like `arr[x][1] = arr[y][1]` to be considered self-assignments and
therefore removed.

## Details

Change `comparePaths` to not regress the 'overlap' state from `maybe`
back to `yes` when the compared array indices are the same, which led
to expressions of the same shape where the outermost array access uses
the same static index to being treated as referring to the same
location (even when they don't).

`injectdestructors` uses the `aliasanalysis` results to remove self-
assignments, causing the erroneous removal of assignments. All other
uses of `aliasanalysis` are for disabling optimizations, so
observable behaviour was not affected there.

---------

Co-authored-by: Saem Ghani <[email protected]>
  • Loading branch information
zerbina and saem authored Jun 27, 2024
1 parent e0ad8b1 commit a21c4db
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 2 deletions.
9 changes: 7 additions & 2 deletions compiler/sem/aliasanalysis.nim
Original file line number Diff line number Diff line change
Expand Up @@ -205,9 +205,14 @@ proc compare*(body: MirTree, a, b: Path): CmpLocsResult =
break

of pikIndex:
overlaps = sameIndex(na, nb)
if overlaps == no:
case sameIndex(na, nb)
of no:
overlaps = no
break
of yes:
discard "don't change back to 'yes'"
of maybe:
overlaps = maybe

inc i

Expand Down
28 changes: 28 additions & 0 deletions tests/assign/tarray_expr_assignment.nim
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
discard """
description: '''
Regression test for an alias-analysis bug that caused expressions involving
array-subscript expressions to erroneously be eliminated in some cases
'''
targets: c js vm
"""

type
Object = object
val: int
Type = array[2, array[2, Object]]

# important: the bug only affected code in procedures that work with destructor-
# having types
proc `=destroy`(x: var Object) =
discard

proc test(x, y: int) {.noinline.} =
var a = [[Object(val: 1), Object(val: 2)], [Object(val: 3), Object(val: 4)]]
# whether the two expressions refer to the same location is unknown; the
# assignment must not be removed
a[x][1] = a[y][1]
# note: the common trailing [1] sub-expressions are important for this test

doAssert a[0][1].val == 4

test(0, 1)

0 comments on commit a21c4db

Please sign in to comment.