Be somewhat clearer about upper bits in the codegen. #1555
Conversation
ltratt
force-pushed
the
more_undefined_bits
branch
from
4a92eb6 to
af8b6ac
Compare
| //! FIXME: the code generator clobbers registers willy-nilly because at the time of writing we have | ||
| //! a register allocator that doesn't actually use any registers. Later we will have to audit the | ||
| //! backend and insert register save/restore for clobbered registers. | ||
| //! * When a value is in a register, we make no guarantees about what the upper bits are set to. |
There was a problem hiding this comment.
I know what you mean here, but we should be clear that the upper undefined bits are unused by the value, and that the value is encoded in the lower order bits.
There was a problem hiding this comment.
I wonder if we should also give an example of a typical scenario where zero/sign extend is necessary. That would help us check we are on the same page and help newcomers to understand what we mean.
My understaning is that:
- inputs to operations (where the result would be incorrect if we didn't) must define the undefined bits with sign/zero extend (or equivalent).
- outputs of operations may leave the undefined bits undefined: they may assume that any operation consuming the value will define the upper bits appropriately for the operation in question.
There was a problem hiding this comment.
The phrasing I'm using is a fairly standard phrasing from specifications. I would prefer to follow that lead rather than introduce our own phrasing that people will not be familiar with.
There was a problem hiding this comment.
OK. I would have liked a bit more explanation, but i suppose we have bigger fish to fry.
| @@ -1662,6 +1649,26 @@ impl<'a> Assemble<'a> { | |||
| Ok(()) | |||
| } | |||
|
|
|||
| /// When we're about to make a call, we need to clear upper bits in registers. | |||
There was a problem hiding this comment.
Should we document who/what imposes this on us? Did you say it was SysV?
| @@ -1844,7 +1856,7 @@ impl Inst { | |||
| Ok(inst) | |||
| } | |||
|
|
|||
| /// Returns the size of the local variable that this instruction defines (if any). | |||
| /// Returns the size in bytes of the local variable that this instruction defines (if any). | |||
There was a problem hiding this comment.
Should the function be called def_bytew for consistency?
There was a problem hiding this comment.
I would actually prefer it to be just bytew but I have several renaming PRs to do :)
|
LGTM. Just a few doc/naming suggestions. |
|
Please squash. |
ltratt
force-pushed
the
more_undefined_bits
branch
from
0d413ba to
a6a42fe
Compare
|
Squashed. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
ltratt
force-pushed
the
more_undefined_bits
branch
from
a6a42fe to
4b3b0ca
Compare
|
Force pushed a Clippy update. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
This is a bit of a hodgepodge of changes, but there are three major
things:
1. Get rid of all the "weird" sized ints, none of which we see in
practise, and which we thus have low confidence are correct.
2. Simplify some of the zero extension stuff.
3. Zero extend all values to 64 bits before a call. We do this
crudely for now.
ltratt
force-pushed
the
more_undefined_bits
branch
from
4b3b0ca to
a980d52
Compare
|
Force pushed a bit of extra code needed for havlak. Note: the new code correctly masks off upper bits when we spill, which we didn't do before, so this is another case where we now handle |
|
Nice catch. My initial thought was "would it not be the responsibility of the operation using the unspilled value to ensure the undefined bits are sanitised?", but I'm not really sure what the rules are. |
|
deopt, for example, doesn't consider undefined bits, so we want to deal with them here. At least for now. |
That's an interesting observation that I've not considered before: We don't know the assumptions the AOT code generator makes regarding undefined bits. I wonder if our assumptions are compatible with those assumptions. I'll merge this for now, but I think we may have to think hard about this at some point. |
This is a bit of a hodgepodge of changes, but there are three major things: