-
Notifications
You must be signed in to change notification settings - Fork 2
Format improvements (WIP) #63
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
2ed6200
a8b2ad4
e66c92d
f3de737
96c1d4c
7fe31f4
4d089ac
9605ae3
5ff6724
b3e8508
b70b2ed
17ce209
92292d5
3d32e3b
88e5a47
26f14dc
6e5e625
8d9e7a8
7a8dd56
4e2e6a2
ac21a91
fba8534
e04f291
20a4da0
b2dcd1c
7778c45
21c30dd
3bdc5fa
e9c3d28
471b51d
1a5964e
8eb4c4b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,7 +31,7 @@ pub struct InstrAbi { | |
| /// | ||
| /// By this notion, [`ArgEncoding`] tends to be more relevant for immediate/literal arguments, while | ||
| /// [`ScalarType`] tends to be more relevant for variables. | ||
| #[derive(Debug, Clone, PartialEq, Eq, Hash)] | ||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub enum ArgEncoding { | ||
| /// `S`, `s`, or `c` in mapfile. Integer immediate or register. Displayed as signed. | ||
| /// `U`, `u`, or `b` in mapfile. Integer immediate or register. Displayed as unsigned. | ||
|
|
@@ -47,8 +47,9 @@ pub enum ArgEncoding { | |
| JumpOffset, | ||
| /// `t` in mapfile. Max of one per instruction, and requires an accompanying `o` arg. | ||
| JumpTime, | ||
| /// `_` in mapfile. Unused 1-byte space. | ||
| Padding, | ||
| /// `_` in mapfile. Unused 4-byte space. | ||
| /// `-` in mapfile. Unused 1-byte space. | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. forgot to mention before. The first line of a docstring is special. It shows up as the summary of the item. |
||
| Padding { size: u8 }, | ||
| /// `f` in mapfile. Single-precision float. | ||
| Float { immediate: bool }, | ||
| /// `z(bs=<int>)`, `m(bs=<int>;mask=<int>,<int>,<int>)`, or `m(len=<int>;mask=<int>,<int>,<int>)` in mapfile. | ||
|
|
@@ -66,7 +67,7 @@ pub enum ArgEncoding { | |
| }, | ||
| } | ||
|
|
||
| #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] | ||
| #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)] | ||
| pub enum StringArgSize { | ||
| /// A string arg that uses `len=`. | ||
| /// | ||
|
|
@@ -93,7 +94,9 @@ impl ArgEncoding { | |
| Self::Integer { size: _, .. } => "integer", | ||
| Self::JumpOffset => "jump offset", | ||
| Self::JumpTime => "jump time", | ||
| Self::Padding => "padding", | ||
| Self::Padding { size: 4 } => "dword padding", | ||
| Self::Padding { size: 1 } => "byte padding", | ||
| Self::Padding { size: _ } => "padding", | ||
| Self::Float { .. } => "float", | ||
| Self::String { .. } => "string", | ||
| } | ||
|
|
@@ -125,18 +128,23 @@ impl ArgEncoding { | |
| } | ||
|
|
||
| pub fn contributes_to_param_mask(&self) -> bool { | ||
| !matches!(self, Self::Padding) | ||
| !matches!(self, Self::Padding { .. }) | ||
| } | ||
|
|
||
| pub fn is_immediate(&self) -> bool { | ||
| matches!(self, | ||
| pub fn is_always_immediate(&self) -> bool { | ||
| match self { | ||
| | Self::String { .. } | ||
| | Self::JumpOffset | ||
| | Self::JumpTime | ||
| | Self::Padding | ||
| | Self::Padding { .. } | ||
| | Self::Integer { immediate: true, .. } | ||
| | Self::Float { immediate: true, .. } | ||
| ) | ||
| => true, | ||
|
|
||
| | Self::Integer { immediate: false, .. } | ||
| | Self::Float { immediate: false, .. } | ||
| => false, | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -207,7 +215,7 @@ impl ArgEncoding { | |
| match self { | ||
| | ArgEncoding::JumpOffset | ||
| | ArgEncoding::JumpTime | ||
| | ArgEncoding::Padding | ||
| | ArgEncoding::Padding { .. } | ||
| | ArgEncoding::Integer { .. } | ||
| => ScalarType::Int, | ||
|
|
||
|
|
@@ -274,15 +282,17 @@ fn int_from_attrs(param: &abi_ast::Param, emitter: &dyn Emitter) -> Result<Optio | |
| hex_radix = true; | ||
| } | ||
|
|
||
| let radix = match hex_radix { | ||
| false => ast::IntRadix::Dec, | ||
| true => ast::IntRadix::Hex, | ||
| }; | ||
|
|
||
| Ok(Some(ArgEncoding::Integer { | ||
| size, | ||
| ty_color: user_ty_color.or(default_ty_color), | ||
| arg0: arg0.is_some(), | ||
| immediate: imm.is_some(), | ||
| format: match hex_radix { | ||
| false => ast::IntFormat { unsigned: unsigned, radix: ast::IntRadix::Dec }, | ||
| true => ast::IntFormat { unsigned: unsigned, radix: ast::IntRadix::Hex }, | ||
| }, | ||
| format: ast::IntFormat { unsigned, radix }, | ||
| })) | ||
| }) | ||
| } | ||
|
|
@@ -351,7 +361,8 @@ fn other_from_attrs(param: &abi_ast::Param, _emitter: &dyn Emitter) -> Result<Op | |
| match param.format_char.value { | ||
| 'o' => Ok(Some(ArgEncoding::JumpOffset)), | ||
| 't' => Ok(Some(ArgEncoding::JumpTime)), | ||
| '_' => Ok(Some(ArgEncoding::Padding)), | ||
| '_' => Ok(Some(ArgEncoding::Padding { size: 4 })), | ||
| '-' => Ok(Some(ArgEncoding::Padding { size: 1 })), | ||
| _ => Ok(None), | ||
| } | ||
| } | ||
|
|
@@ -407,8 +418,8 @@ fn abi_to_signature(abi: &InstrAbi, abi_span: Span, ctx: &mut CompilerContext<'_ | |
| | ArgEncoding::JumpTime | ||
| => Info { ty: ScalarType::Int, default: None, reg_ok: false, ty_color: None }, | ||
|
|
||
| | ArgEncoding::Padding | ||
| => Info { ty: ScalarType::Int, default: Some(sp!(0.into())), reg_ok: true, ty_color: None }, | ||
| | ArgEncoding::Padding { .. } | ||
| => Info { ty: ScalarType::Int, default: Some(sp!(0.into())), reg_ok: false, ty_color: None }, | ||
ExpHP marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| | ArgEncoding::Float { .. } | ||
| => Info { ty: ScalarType::Float, default: None, reg_ok: true, ty_color: None }, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -253,7 +253,7 @@ impl IntrinsicAbiHelper<'_> { | |
| } | ||
|
|
||
| fn find_and_remove_padding(&self, arg_encodings: &mut Vec<(usize, &ArgEncoding)>) { | ||
| arg_encodings.retain(|(_, enc)| !matches!(*enc, ArgEncoding::Padding)); | ||
| arg_encodings.retain(|(_, enc)| !matches!(*enc, ArgEncoding::Padding { .. })); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was also simplified by changing padding to be format letters instead of optional args |
||
| } | ||
|
|
||
| fn find_and_remove_jump(&self, arg_encodings: &mut Vec<(usize, &ArgEncoding)>) -> Result<(usize, abi_parts::JumpArgOrder), Diagnostic> { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -224,19 +224,34 @@ fn decode_args_with_abi( | |
|
|
||
| let reg_style = hooks.register_style(); | ||
| for (arg_index, enc) in siggy.arg_encodings().enumerate() { | ||
| // Padding produces values for the sake of verifying the bytes are 0. TODO: Implement! | ||
| // They're filtered out later on after dealing with @arg0 in the argument-raising pass. | ||
| if let ArgEncoding::Padding { size } = enc { | ||
| decrease_len(emitter, &mut remaining_len, *size as usize)?; | ||
| let raw_value = match size { | ||
| 1 => args_blob.read_u8().expect("already checked len") as i32, | ||
| 4 => args_blob.read_u32().expect("already checked len") as i32, | ||
| _ => unreachable!(), | ||
| }; | ||
| args.push(SimpleArg { value: ScalarValue::Int(raw_value), is_reg: false } ); | ||
| continue; | ||
| } | ||
| let ref emitter = add_argument_context(emitter, arg_index); | ||
|
|
||
| // TODO: Add a way to fallback to @mask for | ||
| // "bad" mask bits to allow roundtripping | ||
| let can_be_param = if enc.contributes_to_param_mask() { | ||
| let value = !enc.is_immediate() && param_mask & 1 == 1; | ||
| let value = !enc.is_always_immediate() && param_mask & 1 == 1; | ||
| param_mask >>= 1; | ||
| value | ||
| } else { | ||
| false | ||
| }; | ||
|
|
||
| let value = match *enc { | ||
| | ArgEncoding::Padding { .. } | ||
| => unreachable!(), | ||
|
|
||
| | ArgEncoding::Integer { arg0: true, .. } | ||
| => { | ||
| // a check that non-timeline languages don't have timeline args in their signature | ||
|
|
@@ -250,7 +265,7 @@ fn decode_args_with_abi( | |
| | ArgEncoding::Integer { arg0: false, size: 4, format: ast::IntFormat { unsigned: false, radix: _ }, .. } | ||
| => { | ||
| decrease_len(emitter, &mut remaining_len, 4)?; | ||
| ScalarValue::Int(args_blob.read_i32().expect("already checked len") as i32) | ||
| ScalarValue::Int(args_blob.read_i32().expect("already checked len")) | ||
| }, | ||
|
|
||
| | ArgEncoding::Integer { arg0: false, size: 2, format: ast::IntFormat { unsigned: false, radix: _ }, .. } | ||
|
|
@@ -277,7 +292,6 @@ fn decode_args_with_abi( | |
| ScalarValue::Int(args_blob.read_u16().expect("already checked len") as i32) | ||
| }, | ||
|
|
||
| | ArgEncoding::Padding | ||
| | ArgEncoding::Integer { arg0: false, size: 1, format: ast::IntFormat { unsigned: true, radix: _ }, .. } | ||
| => { | ||
| decrease_len(emitter, &mut remaining_len, 1)?; | ||
|
|
@@ -556,7 +570,7 @@ impl AtomRaiser<'_, '_> { | |
| // | ||
| // IMPORTANT: this is looking at the original arg list because the new lengths may differ due to arg0. | ||
| let mut arg_iter = abi.arg_encodings(); | ||
| raised_args.retain(|_| !matches!(arg_iter.next().unwrap(), ArgEncoding::Padding)); | ||
| raised_args.retain(|_| !matches!(arg_iter.next().unwrap(), ArgEncoding::Padding { .. })); | ||
|
|
||
| Ok(RaisedIntrinsicParts { | ||
| opcode: Some(instr.opcode), | ||
|
|
@@ -665,7 +679,7 @@ impl AtomRaiser<'_, '_> { | |
| | ArgEncoding::Integer { ty_color: None, format, .. } | ||
| => Ok(ast::Expr::LitInt { value: raw.expect_int(), format: *format }), | ||
|
|
||
| | ArgEncoding::Padding | ||
| | ArgEncoding::Padding { .. } | ||
| => Ok(ast::Expr::from(raw.expect_int())), | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if this is unreachable now?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's definitely not unreachable since the padding args are only dropped after being raised thanks to some sort of logic with |
||
|
|
||
| | ArgEncoding::Integer { ty_color: Some(ty_color), format, .. } | ||
|
|
@@ -677,7 +691,7 @@ impl AtomRaiser<'_, '_> { | |
| &lookup_table, | ||
| raw.expect_int(), | ||
| ty_color, | ||
| format, | ||
| *format, | ||
| )) | ||
| } | ||
|
|
||
|
|
@@ -750,7 +764,7 @@ impl AtomRaiser<'_, '_> { | |
| } | ||
| } | ||
|
|
||
| fn raise_to_possibly_named_constant(names: &IdMap<i32, Sp<Ident>>, id: i32, ty_color: &TypeColor, format: &ast::IntFormat) -> ast::Expr { | ||
| fn raise_to_possibly_named_constant(names: &IdMap<i32, Sp<Ident>>, id: i32, ty_color: &TypeColor, format: ast::IntFormat) -> ast::Expr { | ||
| match names.get(&id) { | ||
| Some(ident) => { | ||
| match ty_color { | ||
|
|
@@ -762,7 +776,7 @@ fn raise_to_possibly_named_constant(names: &IdMap<i32, Sp<Ident>>, id: i32, ty_c | |
| }, | ||
| } | ||
| }, | ||
| None => ast::Expr::LitInt { value: id, format: *format }, | ||
| None => ast::Expr::LitInt { value: id, format: format }, | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might prefer
signedrather thanunsignedto avoid double negativesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with
unsignedsince it's more of an exception to the norm than anything. I tend to structure my bools withtrueas the "do something different" value so that they don't need to be negated whenever being read, but I guess that doesn't matter as much withmatchsince you have to be explicit in both cases.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trueas "do something different" makes sense in C-likes where it's easy to zero-initialize things. It doesn't make so much sense in rust.