Skip to content

Conversation

@zerbina
Copy link
Collaborator

@zerbina zerbina commented May 19, 2025

Summary

Fix various bugs with skully. For the most part, they were about
incorrect code being emitted, resulting in misbehaving programs.

Details

  • fix size/alignment of embedded records (has no observable effect on
    compilation, at the moment)
  • fix nil not being handled in constants, causing a crash
  • fix incorrect code being emitted for closure equality tests
  • fix pointer-location default initialization using an integer zero
  • fix zero being used instead of Nil in various places
  • fix wrong conversion operators being used for integer conversion
  • fix integer extension conversion operators not being emitted
  • fix ord not being translated to a conversion
  • fix code emitted for mDestroy accessing the wrong field
  • fix wrong code being emitted for cast where at least one operand is
    a float or imported type

zerbina added 11 commits May 19, 2025 18:27
Both embedded unions and records had incorrect sizes and alignments
computed for them. None of the current passes cared, hence this causing
no problems, but the LLVM code generator does care.

The size and alignment for embedded records/union is now computed
correctly.
Pointer-like variables were initialized with integer values.
It was treated as a no-op, while it's actually a numeric conversion.
There were various places missed when adjusting skully to the new
pointer types.
* properly handle imported types
* consider float types (which also cannot be truncated/extended)
@zerbina zerbina added the bug Something isn't working label May 19, 2025
@zerbina
Copy link
Collaborator Author

zerbina commented May 19, 2025

Most of these problems were found as part of #146, thanks to the stronger type system (when compared to the VM) of LLVM.

Most of the issues would have also been caught by a type checking pass that skully runs on its output, at least when used in conjunction with building phy with skully. As evidenced by the amount of issues, just running a skully-compiled phy does not provide enough coverage.

Testing skully via unit-like tests is hard, due the MIR + magics having a very large surface area -- there are also no NimSkull-provided facilities for constructing standalone MIR modules (which would be a requirement for testing). Running NimSkull's test suite with skully is also not possible at the moment, due to skully not using the nim CLI.

Ultimately, skully shouldn't have do the translation work it has do now; most of it should be done by the NimSkull compiler. I'm still looking into ways to upstream parts of skully.

@saem saem merged commit 3ea7446 into nim-works:main May 20, 2025
7 checks passed
@zerbina zerbina deleted the skully-fixes branch May 20, 2025 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants