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

mir: lower cast with MIR pass #1411

Open
wants to merge 2 commits into
base: devel
Choose a base branch
from

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented Aug 10, 2024

Summary

When compiling for the C backend, use a MIR pass to lower casts
between non-integer/non-pointer types into nimCopyMem.

Details

  • the lowerCast does the lowering, with the decision whether to use
    nimCopyMem based on the lowered MIR type
  • handling of float and aggregate type casts is removed from cgen
  • cgen used type-punning through a union for such casts, which is
    possible, but not as easy to do with a MIR pass, so nimCopyMem
    (i.e., memcpy) is used instead

The copy always copies size-of-destination number of bytes.


Notes for Reviewers

  • block copies are a common operation, and my plan is to introduce a dedicated
    MemCopy MIR operator at one point
  • casting between different-sized types should become an error

Summary
=======

When compiling for the C backend, use a MIR pass to lower `cast`s
between non-integer/non-pointer types into `nimCopyMem`.

Details
=======

* the `lowerCast` does the lowering, with the decision whether to use
  `nimCopyMem` based on the lowered MIR type
* handling of `float` and aggregate type casts is removed from `cgen`
* `cgen` used type-punning through a union for such casts, which is
  possible, but not as easy to do with a MIR pass, so `nimCopyMem`
  (i.e., `memcpy`) is used instead

The copy always copies size-of-destination number of bytes.
@zerbina zerbina added refactor Implementation refactor compiler/backend Related to backend system of the compiler labels Aug 10, 2024
@zerbina zerbina added this to the MIR phase milestone Aug 10, 2024
@zerbina
Copy link
Collaborator Author

zerbina commented Aug 10, 2024

There seems to be some mirgen issue where an implicit conversion is kept, leading to the cgen assertion in genSomeCast failing.

@alaviss
Copy link
Contributor

alaviss commented Aug 10, 2024

  • casting between different-sized types should become an error

That doesn't sound good to me, truncation is a perfectly valid use case for different size casts, especially when integer narrowing conversions are range checked and we wanted to bypass that.

EDIT: It does make some sense to ban such conversions between objects, though. But then we do acknowledge that cast is dangerous/unsafe, so I'm not sure if forcing devs to copyMem manually is justified.

@zerbina
Copy link
Collaborator Author

zerbina commented Aug 11, 2024

That doesn't sound good to me, truncation is a perfectly valid use case for different size casts, especially when integer narrowing conversions are range checked and we wanted to bypass that.

You're right, my statement was too broad. What happens in the truncation case is easy to explain, understand, implement, and it's useful, and therefore I think it should generally stay allowed. I do think it's better to disallow "expanding" casts, and I'm also starting to think that it'd be better to not make an exception for numeric types.

Ultimately, I think bit-casting from smaller to larger types is less safe (i.e., the excess bytes/bits need to take on some defined or undefined state) than the other two cases (truncating and casting between equal-sized types) and also usually not what one wants when casting, which is why I think it makes sense to disallow it. Making what happens more explicit (either via a copyMem or some safer variation thereof), would be better, in my personal opinion.

It's not a very strong opinion however, and as long as such casts are lowered into memory copies early enough, I don't have too much of a problem with them.

@alaviss
Copy link
Contributor

alaviss commented Aug 11, 2024

I do agree that small -> large is very unsafe. They don't make that much sense for pointers (except for inheritance) and surely don't make much sense for a bit cast.

I'm also starting to think that it'd be better to not make an exception for numeric types.

There's one edge case that is a bit lengthier to do if this is not allowed: conversion between int -> uint of a larger size without sign extension. Though obviously the overhead is just int16 - cast -> uint16 -> uint64 to handle that case.

One of these days we might have to reformulate how integer conversion is done to make them easier to use when implementing algorithms, but that's a conversation for later.

@mrgaturus
Copy link
Contributor

mrgaturus commented Sep 13, 2024

casting between different-sized types should become an error

i disagree reduce degrees of freedom to something that is unsafe at every way. I think to reduce unsafety on small -> bigger casting we can zero-fill first if those casts are performed. in the case of integers, cpus just zero extends numbers to bigger registers.

my problem with cast[T] was about how it can be confused with a simple and safe conversions T(), i think in future manual rewrites we should reforce cast and conversions differences for example float <-> integer conversion vs casting

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/backend Related to backend system of the compiler refactor Implementation refactor
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants