Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes #22286; enforce Non-var T destructors by nimPreviewNonVarDestructor #22975

Merged
merged 13 commits into from
Nov 25, 2023

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Nov 21, 2023

fixes #22286
ref https://forum.nim-lang.org/t/10642

For backwards compatibilities, we might need to keep the changes under a preview compiler flag. Let's see how many packags it break.

TODO in the following PRs

  • Turn the var T destructors warning into an error with nimPreviewNonVarDestructor

ringabout added a commit to ringabout/weave that referenced this pull request Nov 21, 2023
@ringabout ringabout changed the title prepare to enforce Non-var T destructors fixes #22286; enforce Non-var T destructors by nimPreviewNonVarDestructor Nov 22, 2023
@ringabout ringabout marked this pull request as ready for review November 22, 2023 14:44
@ringabout
Copy link
Member Author

Ops, forgot to add test cases, will push them tomorrow

@ringabout ringabout marked this pull request as draft November 23, 2023 04:14
@ringabout
Copy link
Member Author

The failure of tests/arc/thard_alignment.nim seems to be quite nasty... I'm debugging it on my local branch.

@ringabout
Copy link
Member Author

ringabout commented Nov 23, 2023

It seems that my change may not be the culprit of the failure of tests/arc/thard_alignment.nim.

This is a reduced case:

{.passC: "-march=native".}

proc isAlignedCheck(p: pointer, alignment: int) = 
  doAssert (cast[uint](p) and uint(alignment - 1)) == 0

proc isAlignedCheck[T](p: ref T, alignment: int) = 
  isAlignedCheck(cast[pointer](p), alignment)

type
  m256d {.importc: "__m256d", header: "immintrin.h".} = object

proc set1(x: float): m256d {.importc: "_mm256_set1_pd", header: "immintrin.h".}
func `+`(a,b: m256d): m256d {.importc: "_mm256_add_pd", header: "immintrin.h".}
proc toString(a: m256d): string = discard


var xx = new(m256d)
xx[] = set1(10)
isAlignedCheck(xx, alignOf(m256d))
echo toString(xx[])

When defining a non-var function for m256d, it segfaulted on Windows. Not sure whether {.passC: "-march=native".} is to blame or not.

So It could cause problems for the non-var destructor that is lifted for m256d.

@ringabout ringabout marked this pull request as ready for review November 23, 2023 12:43
@ringabout
Copy link
Member Author

As https://discord.com/channels/371759389889003530/768367394547957761/1177513506610880542 said, the varness change of destructors might affect alignments with -march=native.

ringabout and others added 2 commits November 25, 2023 13:24
Co-authored-by: Andreas Rumpf <rumpf_a@web.de>
Co-authored-by: Andreas Rumpf <rumpf_a@web.de>
@Araq Araq added the merge_when_passes_CI mergeable once green label Nov 25, 2023
@Araq Araq merged commit 379299a into devel Nov 25, 2023
19 checks passed
@Araq Araq deleted the pr_generics_bind branch November 25, 2023 17:27
Copy link
Contributor

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from 379299a

Hint: mm: orc; opt: speed; options: -d:release
176860 lines; 6.697s; 766.102MiB peakmem

narimiran pushed a commit that referenced this pull request Jul 5, 2024
…uctor` (#22975)

fixes #22286
ref https://forum.nim-lang.org/t/10642

For backwards compatibilities, we might need to keep the changes under a
preview compiler flag. Let's see how many packags it break.

**TODO** in the following PRs

- [ ] Turn the `var T` destructors warning into an error with
`nimPreviewNonVarDestructor`

---------

Co-authored-by: Andreas Rumpf <rumpf_a@web.de>
(cherry picked from commit 379299a)
narimiran pushed a commit that referenced this pull request Jul 5, 2024
…uctor` (#22975)

fixes #22286
ref https://forum.nim-lang.org/t/10642

For backwards compatibilities, we might need to keep the changes under a
preview compiler flag. Let's see how many packags it break.

**TODO** in the following PRs

- [ ] Turn the `var T` destructors warning into an error with
`nimPreviewNonVarDestructor`

---------

Co-authored-by: Andreas Rumpf <rumpf_a@web.de>
(cherry picked from commit 379299a)
narimiran pushed a commit that referenced this pull request Jul 9, 2024
…uctor` (#22975)

fixes #22286
ref https://forum.nim-lang.org/t/10642

For backwards compatibilities, we might need to keep the changes under a
preview compiler flag. Let's see how many packags it break.

**TODO** in the following PRs

- [ ] Turn the `var T` destructors warning into an error with
`nimPreviewNonVarDestructor`

---------

Co-authored-by: Andreas Rumpf <rumpf_a@web.de>
(cherry picked from commit 379299a)
narimiran pushed a commit that referenced this pull request Jul 10, 2024
…uctor` (#22975)

fixes #22286
ref https://forum.nim-lang.org/t/10642

For backwards compatibilities, we might need to keep the changes under a
preview compiler flag. Let's see how many packags it break.

**TODO** in the following PRs

- [ ] Turn the `var T` destructors warning into an error with
`nimPreviewNonVarDestructor`

---------

Co-authored-by: Andreas Rumpf <rumpf_a@web.de>
(cherry picked from commit 379299a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge_when_passes_CI mergeable once green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

system.=destroy does not accept non-var types
2 participants