Added Cmm_helpers.Scalar_type #3423
Conversation
jvanburen
requested review from
mshinwell and
Gbury
and removed request for
mshinwell
Selection Change CheckThis PR modifies the original selection pass (targeting Mach). |
jvanburen
force-pushed
the
cmm-refactor-unboxed-fields
branch
from
16f1b23 to
443a81c
Compare
jvanburen
force-pushed
the
cmm-scalar-type
branch
from
39828c2 to
ae1f27d
Compare
jvanburen
force-pushed
the
cmm-refactor-unboxed-fields
branch
from
443a81c to
2f5d60b
Compare
jvanburen
force-pushed
the
cmm-scalar-type
branch
from
ae1f27d to
d4a7719
Compare
jvanburen
force-pushed
the
cmm-scalar-type
branch
2 times, most recently
from
1d28ccf to
e856bc9
Compare
TheNumbat
changed the base branch from
main
to
refactor-cmm-for-more-integer-widths
I noticed that the generated |
|
Should we e.g. examine the actual generated assembly for a few executables before and after this change? I think probably yes. |
jvanburen
force-pushed
the
cmm-scalar-type
branch
from
baa577d to
bb2d88e
Compare
| let nativeint = Untagged Integer.nativeint | ||
|
|
||
| let[@inline] untagged = function | ||
| | Untagged t -> t |
There was a problem hiding this comment.
Are there legitimate uses where we try and untag an already untagged integral type ? That seems like a bug to me at first glance, so unless we have some actual uses cases (and they are correct), we should probably raise a fatal_error here since this may indicate a bug elsewhere ?
There was a problem hiding this comment.
Yes, this is so that you can cast to the untagged representation of a type
There was a problem hiding this comment.
it's not for asserting that we know a type is untagged
There was a problem hiding this comment.
Yes, this is so that you can cast to the untagged representation of a type
When do we legitimately need to "cast to the untagged representation of a type" when that type is already untagged ?
Put another way: I'm arguing that this line should read Untagged _ -> assert false and that this assert should not trigger since untagging something that is already untagged has no clearly defined semantics and actually indicates a bug/error. Can you give a detailed explanation of at least one case where that line of code is effectively used, and why do we need it / why the code is better (or simpler) that way ?
There was a problem hiding this comment.
From what I read, the only use of that function is in cast_cast_without_losing_information , where the actual information that is of interest is the number of bits actually used to store information, so I think it should be better to rearrange the code a bit to not need this function (and instead use a function that returns the actually use-for-information bit-width for instance).
There was a problem hiding this comment.
it's useful b/c you can static_cast or conjugate to untagged kind , and it's just a no-op if the integer is already untagged.
backend/cmm_helpers.ml
Outdated
| Misc.fatal_errorf | ||
| "static_cast: casting floats to unsigned values is undefined" | ||
| | Signed -> | ||
| (* we can truncate, but we don't want to promote *) |
There was a problem hiding this comment.
I clarified the comments
|
Also: there's a CI failure that seems related to this PR: |
jvanburen
force-pushed
the
cmm-scalar-type
branch
3 times, most recently
from
8a57568 to
5a2037e
Compare
This will be used by a future pr to simplify casting without impacting performance as much
jvanburen
force-pushed
the
cmm-scalar-type
branch
from
5a2037e to
8fa9539
Compare
jvanburen
force-pushed
the
cmm-scalar-type
branch
from
8fa9539 to
6dce5c0
Compare
…ng between integers types of different widths and signedness. This is in preparation for adding unboxed small integer types.
jvanburen
force-pushed
the
cmm-scalar-type
branch
from
6dce5c0 to
e13ed4b
Compare
Cmm_helpers.Scalar_typeprovides utilities for converting between integers types of different widths and signedness. This is in preparation for adding unboxed small integer types.