-
Notifications
You must be signed in to change notification settings - Fork 16
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
Implement assembly for non-vector z16 instructions #512
base: main
Are you sure you want to change the base?
Conversation
src/az390.java
Outdated
if (bal_op.equals("NNPA")){ // DSH #414 z16 | ||
get_hex_zero(2); | ||
} else if (bal_op.equals("PALB") // RPI 277 |
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.
Couldn't this be simplified to
if (bal_op.equals("NNPA")){ // DSH #414 z16
|| (bal_op.equals("PALB") // RPI 277```
That is, in pseudocode, "if ("NNPA" or "PALB" or "PCKMO" or "PCC") then get_gex_zero(2)"
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.
Agreed. Changed as per your suggestion.
skip_comma(); | ||
get_hex_reg(); // M4 | ||
} else { | ||
if ((tz390.op_code[bal_op_index].length() == 5)) { |
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 want to add a comment explaining what's being checked here. Not at all clear to me.
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.
Agreed. Pretty convoluted code. PoP says masks are specified as M6,M5,M4 which is at odds with the comments.
It look like the program is testing the mask operands one by one, but M6 and M5 are obligatory. Or do they default to zero when omitted? I tried to understand in order to add comments - but I'm worried I'd get it wrong.
src/az390.java
Outdated
} else if (fp_text.toUpperCase().equals("(INF)")){ // #423 | ||
switch (tz390.fp_type){ // gen (min) hex for tz390.fp_type | ||
case 0: // tz390.fp_db_type s1,e11,m52 with assumed 1 | ||
dc_hex = "7FF0000000000000"; | ||
break; | ||
case 1: // tz390.fp_dd_type s1,cf5,bxcf8,ccf50 | ||
dc_hex = "EEEEEEEEEEEEEEEE"; | ||
break; |
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.
Following comment copied from what I added back in August 2022 to Don's initial PR. Also, read my other comments in that PR.
- INF, NAN, QNAN, and SNAN only apply to binary and decimal floating point. See the HLASM Language Reference manual; p156 of the -9 version. It also shows that DMIN is another special value. MAX, MIN and DMIN apply to all 3 floating point types. The az390 code does NOT implement "(DMIN)". Should it?
- Why all the "EEE...EEE" values? Why not the correct values? John Ehrman's assembler text has tables that show the exact values. For example, the value for INF for DD is "7800000000000000". See Table 325 in Section 33.5 of the book for all the DFP values. See Table 276 of Section 34.2 for BFP values. See Table 238 in Section 33.4.2 for the MAX, MIN and DMIN values for HFP. Note there are no HFP INF, NAN, QNAN or SNAN values.
- Unrelated, but some of the MIN values for HFP are incorrect and the MIN values for DFP appear to be DMIN values. Please doublecheck all values against the tables mentioned above.
- Not sure why you added INF,NAN, QNAN and SNAN for HFP. Shouldn't these generate assembly errors?
- You should replace eachl "EEE...EEE" value with the corresponding correct value.
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.
Agreed on all 5 points.
I think I have addressed all of them.
…ding zero values. This is an incomplete set of changes since at least five RT programs are now broken
This is an intermediate version - for now it just contains the results of the merge. There was a conflict with John's changes for issue #451
I am sharing this version to allow you all to double-check on the fp constants I definded in tz390