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.

Sign up for GitHub

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

Jump to bottom

Deal properly with undefined bits #1554

Merged
merged 2 commits into from
Jan 20, 2025
Merged

Deal properly with undefined bits #1554

merged 2 commits into from
Jan 20, 2025

Conversation

ltratt
Copy link
Contributor

@ltratt ltratt commented Jan 20, 2025

This PR fixes two cases where we checked whole bytes, instead of a single bit, in generated code.

ltratt added 2 commits January 20, 2025 12:27
@ltratt
Test comparisons using bt.
30a7d45 
Previously we checked `i1`s for guards using `cmp` i.e. we compared
bytes instead of bits, breaking the "the upper bits are undefined"
invariant. This commit does two things:

  1. It changes the parts related to icmp to make clear that we've only
     tested the 8/16/32/64 bit cases. In particular, I'm not sure
     if the 1..=7 cases were, previously, broken or not. Since we don't
     seem to encounter them I'd rather `todo` them than try and think
     about them now.

  2. It changes guard generation to use `bt` rather than `cmp` i.e. we
     only check the top bit instead of a whole byte.
@ltratt
Select must have an i1 conditional in LLVM IR.
add873d 
This means that we should use `bt`, not `cmp`. Update the
well-formedness checker to enforce this, which also uncovered a broken
test.
@vext01
Copy link
Contributor

vext01 commented Jan 20, 2025

About the first commit:

we compared bytes instead of bits, breaking the "the upper bits are undefined" invariant.

Suppose we have two i1s, %lhs and %rhs and we want to compare for equality.

And let's assume that these values are stored in registers and the registers contain 0x1 and 0xffffffffffffffff. Since only the bottom bit is defined, these numbers both encode 1i1 (when the number is interpreted unsigned).

What the current codegen will do is sign or zero extend (depending on the signedness of the operation) the n-bit values (here n=1) up to a 64-bit value. This is done with self.{sign,zero}_extend_to_reg64(reg, n). Here n tells the routine how many bits to consider undefined so that it can replace them with either zeros or ones (again, depending on whether the comparison is signed or not).

In this example eq is an unsigned comparison, so %lhs is zero extended and remains 0x1 and %rhs is zero extended to 0x1. Then it is the zero extended values that are compared. Here both values are the same 0x1 and thus the comparison is computed correctly to be true.

All this to say: I disagree that the current code breaks the "the upper bits are undefined" invariant. It works by ensuring the undefined bits are defined before doing the 64-bit comparison. Or at least that was the intention when I wrote this code. Have I missed something?

I do agree that special casing i1 to a bit check is a good optimisation though.

@ltratt
Copy link
Contributor Author

ltratt commented Jan 20, 2025

I claim that it is likely that the bug fixed in #1553 was caused by the flaws in this PR. Remember that #1553 fixed a literal crash.

@ltratt
Copy link
Contributor Author

ltratt commented Jan 20, 2025

Actually, notice that cg_guard did not sign/zero extend/truncate! So it literally is broken before this commit, because it compared a whole byte, and did not do anything with the upper bits.

@vext01
Copy link
Contributor

vext01 commented Jan 20, 2025

I'm happy with the first commit if we can remove the part in the commit message saying that the code was invalidating the undefinedness guarantees. I think my explanation above shows that the codegen is actually quite careful to ensure that it isn't invalidated.

@vext01
Copy link
Contributor

vext01 commented Jan 20, 2025

Hang on, we raced.

@vext01
Copy link
Contributor

vext01 commented Jan 20, 2025

OK, now we are getting somewhere. It looks like cg_guard is indeed incorrect. I only verified the general cmp case (which I still believe to be correct).

vext01
vext01 reviewed Jan 20, 2025
View reviewed changes
ykrt/src/compile/jitc_yk/codegen/x64/mod.rs
@@ -2492,8 +2492,16 @@ impl<'a> Assemble<'a> {
RegConstraint::Input(inst.falseval(self.m)),
],
);
dynasm!(self.asm ; cmp Rb(cond_reg.code()), 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this one is comparing undefined bits too!

@vext01
Copy link
Contributor

vext01 commented Jan 20, 2025

In summary:

  • cg_guard was reading undefined bits
  • cg_select was too
  • cg_icmp was correct, but we've optimised it a bit.

@vext01 vext01 added this pull request to the merge queue Jan 20, 2025
Merged via the queue into ykjit:master with commit a70179c Jan 20, 2025
2 checks passed
@ltratt ltratt deleted the undefined_bits branch January 24, 2025 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vext01 vext01 vext01 left review comments

Assignees

@vext01 vext01

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ltratt @vext01