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

unify put mnemonic; always value first #77

Merged
merged 6 commits into from
Jun 14, 2023
Merged

Conversation

6293
Copy link
Contributor

@6293 6293 commented Jun 12, 2023

Closes #75

@codecov-commenter
Copy link

codecov-commenter commented Jun 12, 2023

Codecov Report

Merging #77 (260a9f5) into master (f57d933) will increase coverage by 0.0%.
The diff coverage is 52.4%.

❗ Current head 260a9f5 differs from pull request most recent head 4caac4a. Consider uploading reports for the commit 4caac4a to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@          Coverage Diff           @@
##           master     #77   +/-   ##
======================================
  Coverage    41.1%   41.1%           
======================================
  Files          19      19           
  Lines        4542    4547    +5     
======================================
+ Hits         1865    1868    +3     
- Misses       2677    2679    +2     
Impacted Files Coverage Δ
src/data/byte_str.rs 29.9% <0.0%> (-0.3%) ⬇️
src/isa/bytecode.rs 17.4% <ø> (ø)
src/isa/instr.rs 10.5% <0.0%> (ø)
src/isa/exec.rs 59.9% <70.0%> (ø)
src/reg/core_regs.rs 55.7% <100.0%> (+0.2%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Comment on lines +696 to +701
write!(f, "\n{}S-REG:{}\t", sect, reset)?;
for i in 0..16 {
if let Some(ref v) = self.s16[i] {
write!(f, "{}s16{}[{}{:02}{}]={}{}{}\n\t", reg, eq, reset, i, eq, val, v, reset)?;
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

put   100,r8192[1];
put   2,r8192[2];
put   18,a16[1];
put   2,a16[2];
put   "aaa",s16[1];
put   "aaa",s16[2];

would be displayed as

CTRL:   st0=true cy0=0 ca0=8 cl0=~ cp0=0 
                cs0=0 @ alien_vista_plaza_11111111111111111111111111111111
                   
A-REG:  a16[01]=0012h   a16[02]=0002h   
                
F-REG:  
R-REG:  r8192[01]=64h
                
S-REG:  s16[01]="aaa"
        s16[02]="aaa"

@6293 6293 force-pushed the bytes-put branch 4 times, most recently from 260a9f5 to 4caac4a Compare June 13, 2023 13:58
Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

I propose to make the PR not-API breaking (see my comment).

Also, what do you think, should we use left-to-right argument order (like now, with value going first), or do a X86-assembly way where the destination goes first?

@@ -1102,7 +1102,7 @@ impl Bytecode for BytesOp {
W: Write,
{
match self {
BytesOp::Put(reg, bytes, _) => {
BytesOp::Put(bytes, reg, _) => {
Copy link
Member

Choose a reason for hiding this comment

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

I am afraid changing in BytesOp::Put argument order is a breaking change which makes this PR impossible to release without a major version bump (while it will be nice to have it as a fix instead).

@6293
Copy link
Contributor Author

6293 commented Jun 14, 2023

Also, what do you think, should we use left-to-right argument order (like now, with value going first), or do a X86-assembly way where the destination goes first?

We have both styles for now (mov accepts source first while extr is dest first). So we can align to the "major" X86 style

@dr-orlovsky
Copy link
Member

@6293 This alignment looks like a separate PR. But at least we can update this PR to match the future alignment. So if you are ok with x86 style (destination first, then source), can you pls update it here?

@dr-orlovsky
Copy link
Member

It can be that Intel style BTW is not the best one so we might want to stick to more natural (for english) left-to-right order: https://stackoverflow.com/questions/67597388/correct-order-of-source-and-destination-in-assembly-language

Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

LGTM except that we still need not to change BytesOp variants - otherwise we would break bytecode compatibility (and would also need a major version release).

So yes, the enum variants will not match the mnemonic representation. Sad, but otherwise too breaking downstream.

@6293
Copy link
Contributor Author

6293 commented Jun 14, 2023

LGTM except that we still need not to change BytesOp variants - otherwise we would break bytecode compatibility (and would also need a major version release).

the change on BytesOp::Put has been reverted in the latest commit

@6293 6293 requested a review from dr-orlovsky June 14, 2023 10:35
Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

ACK 6cae121

@dr-orlovsky dr-orlovsky merged commit db921e0 into AluVM:master Jun 14, 2023
@dr-orlovsky
Copy link
Member

Thank you very much, great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make put mnemonic consistent
3 participants