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

Bug in PFPO instruction #407

Closed
gilsonlasco opened this issue Jul 31, 2021 · 24 comments
Closed

Bug in PFPO instruction #407

gilsonlasco opened this issue Jul 31, 2021 · 24 comments
Assignees
Labels
BUG The issue describes likely incorrect product functionality that likely needs corrected.

Comments

@gilsonlasco
Copy link

gilsonlasco commented Jul 31, 2021

Hi everybody.

The PFPO instruction seems to not be working correctly (Hyperion 4.4.9999.0-SDL-gd99fb05 4.4.9999.0, Ubuntu 20.04).

I'm using PFPO to convert an Extended DFP (Decimal Floating-Point) number to a Long HFP (Hexadecimal Floating-Point) number using the two instructions below:

    L    R0,=X'01010A00'      # EXTENDED DFP TO LONG HFP
    PFPO

Starting with the contents of the F4-F6 source operand registers of X'39FFD2B32D873E6EA9DAAD5ABE6B6404' (i.e. decimal 6.283185307179586476925286766559004), the resulting F0 target operand register after executing PFPO is X'40487ED5110B4611' (i.e. decimal 0.283185307179586), when in actuality it should be X'416487ED5110B45F' (i.e. decimal 6.283185307179586).

It seems the integer part of the number (6) is not being returned.

Regards,
Gilson Lasco

@Fish-Git
Copy link
Member

Fish-Git commented Aug 3, 2021

Hi Gilson!

I am sorry you are having trouble with the PFPO instruction. I will try your code on my own system to see if I get the same results as you, and then will report back to you with my results.

If it's a bonafide bug (which I believe it likely is!), then we will try to get it fixed for you right away.

Thank you for reporting it.

@Fish-Git Fish-Git added BUG The issue describes likely incorrect product functionality that likely needs corrected. IN PROGRESS... I'm working on it! (Or someone else is!) Researching... The issue is being looked into or additional information is being gathered/located. labels Aug 3, 2021
@Fish-Git Fish-Git changed the title PFPO seems to be not ok Bug in PFPO instruction Aug 3, 2021
@Fish-Git
Copy link
Member

Fish-Git commented Aug 3, 2021

Gilson,  (@gilsonlasco)

  1. What FPC value are you using? (Floating-Point Control Register)
  2. What CR0 (Control Register 0) value are you using?

Thanks.

@Fish-Git
Copy link
Member

Fish-Git commented Aug 3, 2021

(CC: Bob and Steve) (@rwoodpd @srorso)

Hi Gilson! (@gilsonlasco)

Okay, I'm able to recreate the problem, although my results are slightly different from yours, which concerns me somewhat, since it would seem to imply there is something you're doing that my simple test is not (such as setting a rounding mode perhaps?):

  • PFPO.zip  (Test program source, core file and listing)

Test commands:

sysclear
archlvl     z/Arch
loadcore    "C:\Users\Fish\Documents\Visual Studio 2008\Projects\MyProjects\ASMA-0\PFPO\PFPO.core"
u           200.4A
r           250.50
r           300.20
restart

Test results (i.e. Hercules log):

HHC01603I *
HHC01603I *
HHC01603I *
HHC01603I sysclear
HHC02311I sysclear completed
HHC01603I archlvl     z/Arch
HHC00811I Processor CP00: architecture mode z/Arch
HHC02204I ARCHLVL        set to z/Arch
HHC02204I LPARNUM        set to 1
HHC02204I CPUIDFMT       set to 0
HHC01603I loadcore    "C:\Users\Fish\Documents\Visual Studio 2008\Projects\MyProjects\ASMA-0\PFPO\PFPO.core"
HHC02250I Loading file C:\Users\Fish\Documents\Visual Studio 2008\Projects\MyProjects\ASMA-0\PFPO\PFPO.core to location 0
HHC02249I Operation complete
HHC01603I u           200.4A
HHC02289I R:0000000000000200  EB000270002F LCTLG 0,0,624(0)             load_control_long
HHC02289I R:0000000000000206  B38C0000     EFPC  0                      extract_fpc
HHC02289I R:000000000000020A  50000280     ST    0,640(0,0)             store
HHC02289I R:000000000000020E  E34002900004 LG    4,656(0,0)             load_long
HHC02289I R:0000000000000214  E36002980004 LG    6,664(0,0)             load_long
HHC02289I R:000000000000021A  B3C10044     LDGR  4,4                    load_fpr_from_gr_long_reg
HHC02289I R:000000000000021E  B3C10066     LDGR  6,6                    load_fpr_from_gr_long_reg
HHC02289I R:0000000000000222  E30002780004 LG    0,632(0,0)             load_long
HHC02289I R:0000000000000228  010A         PFPO  ,                      perform_floating_point_operation
HHC02289I R:000000000000022A  B3CD0000     LGDR  0,0                    load_gr_from_fpr_long_reg
HHC02289I R:000000000000022E  E30003100024 STG   0,784(0,0)             store_long
HHC02289I R:0000000000000234  E31003000004 LG    1,768(0,0)             load_long
HHC02289I R:000000000000023A  B9200001     CGR   0,1                    compare_long_register
HHC02289I R:000000000000023E  47700246     BC    7,582(0,0)             branch_on_condition
HHC02289I R:0000000000000242  B2B20250     LPSWE 592(0)                 load_program_status_word_extended
HHC02289I R:0000000000000246  B2B20260     LPSWE 608(0)                 load_program_status_word_extended
HHC01603I r           250.50
HHC02290I A:0000000000000000  K:06
HHC02290I R:0000000000000250  00020001 80000000 00000000 00000000  ................
HHC02290I R:0000000000000260  00020001 80000000 00000000 EEEEEEEE  ................
HHC02290I R:0000000000000270  00000000 00040000 00000000 01010A00  ................
HHC02290I R:0000000000000280  00000000 00000000 00000000 00000000  ................
HHC02290I R:0000000000000290  39FFD2B3 2D873E6E A9DAAD5A BE6B6404  ..K..g.>z.[!.,..
HHC01603I r           300.20
HHC02290I A:0000000000000000  K:06
HHC02290I R:0000000000000300  416487ED 5110B45F 00000000 00000000  ..g....^........
HHC02290I R:0000000000000310  00000000 00000000 00000000 00000000  ................
HHC01603I restart
HHC02228I restart key pressed
HHC00809I Processor CP00: disabled wait state 0002000180000000 00000000EEEEEEEE
HHC01603I *
HHC01603I *
HHC01603I *
HHC01603I r 300.20
HHC02290I A:0000000000000000  K:06
HHC02290I R:0000000000000300  416487ED 5110B45F 00000000 00000000  ..g....^........
HHC02290I R:0000000000000310  40487ED5 110B4612 00000000 00000000   .=N............
HHC01603I *
HHC01603I *
HHC01603I *
HHC01603I fpc
HHC02276I Floating point control register: 00080000
HHC01603I *
HHC01603I *
HHC01603I *

 
Notice that MY results are X'40487ED5110B4612', whereas your results were X'40487ED5110B4611'.

So obviously some type of rounding is going on.

The Principles of Operation mentions the Control Register 0 "AFP-register-control bit" as well as the PFPO instruction's General Purpose Register 0 "RM" (Rounding Method) field (with "0" (zero) defined as "According to current DFP rounding mode in bits 25-27 of the FPC floating-point-control register").

The FPC register bits 25-27 that I am using are 000 (the default) ,which, according to the Principles of Operation, is defined as "Round to nearest with ties to even". Is that the same rounding method you are using?

I suspect your code is not using a default FPC value which is why my result is different from yours. (Mine appears to be using default rounding whereas yours isn't).

Of course neither the chosen rounding method nor the setting of the AFP-register-control bit in CR0 is the root of the problem. I know that! There obviously IS is bug somewhere in our PFPO logic, since, as you pointed out (and as I was able to recreate), the integer part of the number is not being returned. So the fact that a bug does indeed exist is absolutely certain.

BUT... I would very much like to be able to reproduce (recreate) your test results exactly if I could. I don't know much about the PFPO instruction nor Floating-Point in general (both are quite complicated and very close to being beyond my feeble mind's ability to comprehend), but I want to make sure that when I step through Hercules's code, I am following the exact same path as is being followed on your system. That's important IMO if I'm to have any hope of trying to debug this without Bob or Steve's help.

I've asked both Bob Wood and Steve Orso -- two members of our Hercules development team who know Floating-Point WAY better than I do -- to try and help out with this issue if they can (I'd really appreciate it guys! Please?). Hopefully at least one of them will be able to quickly locate and fix the bug for us, but in the mean time, I am going to TRY and locate it on my own. I don't know how much success I'll have but I will make the effort.

So..... What FPC value (DFP rounding method) are you using, Gilson?

Is the AFP-register-control bit set to '1' (on) in Control Register 0?  (I presume it is)

Is there anything else regarding your test environment that I should know about? (that would affect the results?)

Thanks!

@Fish-Git Fish-Git added QUESTION... A question was asked but has not been answered yet, -OR- additional feedback is requested. HELP! Help is needed from someone more experienced or I'm simply overloaded with too much work right now! and removed Researching... The issue is being looked into or additional information is being gathered/located. labels Aug 3, 2021
@gilsonlasco
Copy link
Author

gilsonlasco commented Aug 3, 2021 via email

@Fish-Git
Copy link
Member

Fish-Git commented Aug 4, 2021

I didn't touch CR0 contents.

Maybe you didn't, but maybe whatever operating system you are using did? What operating system are you using? z/OS? z/Linux?

What language are you testing with? C/C++? COBOL? Assembler?

May I see you test program please?

Bits 60-63 of R0 are all set to zero = "According to current DFP rounding mode" before PFPO.

Yes, I can see that. That is not what I need to know.

What I need to know is, what is the current DFP rounding mode? (i.e. what is the value of the FPC?)

In your reply you've mentioned "Round to nearest with ties to even" as the meaning of bits 25-27 of FPC register. I think this rounding mode is not 0, but I'm not sure.

Please see page 9-10 of "z/Architecture Principles of Operation" (SA22-7832-12):

image

Your '...4612' result differs from mine ('...4611') possibly because of that "... with ties to even".

Obviously!

Which I was trying to resolve by asking what rounding mode your FPC was set to.

BUT... it largely does not matter since I am unable to understand any of Bob's code and thus am unable to fix the bug myself.   :-(

So... unless Bob or Steve or someone else more familiar/experienced with Decimal and Hexadecimal Floating-Point can take over for me, I am afraid this bug is going to have to remain unfixed for now.   :-(

Sorry!   :-(

@Fish-Git
Copy link
Member

Fish-Git commented Aug 4, 2021

Gilson,

FYI: I would very much appreciate it if you would not respond/reply to GitHub Issues via email. (*)

I would much prefer that you instead respond/reply directly via the GitHub Issues web page itself:

When you reply directly via their web page, I can make minor edits to your reply so it is more readable (prettier) by editing the fonts being used, etc.

When you reply via email however, I cannot edit your reply, so oftentimes it is much harder (more difficult) to read.

It is up to you whether or not you want to take the time to reply via their web page or continue to reply via email, but I would much rather that you reply directly via their web page instead.

Thanks

(*) GitHub does not support formatting of email replies, making it impossible for me to fix the formatting of a person's reply for readability. Thank you for understanding.

@Fish-Git
Copy link
Member

Fish-Git commented Aug 4, 2021

NOTE:  The below comment is a reformatting of Gilson's original reply which he made via email.
Fish-Git did NOT post the below comment.   Gilson did.


Sorry for the delayed answer, Fish.

You're absolutely right about that rounding mode. I don't use that instruction so often. My bad.

But first things first...

My primary test environment is a z/PDT running z/OS 2.1, where I also have an Hercules 4 installed (seeing the same disk volumes to allow me to recreate situations reported by partners that don't have a z machine). Alternatively, I use IBM Z Dallas ISV Center with z/OS 2.4.

The PFPO is being used on a floating-point routine I wrote in assembler many years ago and decided to modernize. Decimal float is much more precise etc.

I've picked up that values I sent you from a storage dump when recreating a reported issue under Hercules.

Yesterday I wrote a very small asm prog to check, from scratch, what is going on.

Here it is:

FISH     CSECT
FISH     AMODE 31
FISH     RMODE ANY

         BAKR  R14,R0
         LARL  R12,FISH
         USING FISH,R12

         LD    F4,FROMTHIS
         LD    F6,FROMTHIS+8
         L     R0,PARM
         PFPO
         STD   F0,PFPOGAVE

         LG    R0,IWANT
         LG    R1,PFPOGAVE

* ------------------------------------------------------------------- *
* --  NOW YOU HAVE IN R0 WHAT IS EXPECTED FROM PFPO                -- *
* --                  R1 WHAT PFPO DELIVERS                        -- *
* ------------------------------------------------------------------- *
         DC    H'0'     NOW FORCE A S0C1 TO INSPECT GPRS 0-1


PARM     DC    A(X'01010A00')
         DS    0D

         DC    CL8'FROMTHIS'
FROMTHIS DC    LD'6.283185307179586476925286766559002'

         DC    CL8'IWANT'
IWANT    DC     D'6.283185307179586476925286766559002'

         DC    CL8'PFPOGAVE'
PFPOGAVE DC    D'0'

         LTORG

F0       EQU   0
F4       EQU   4
F6       EQU   6

R0       EQU   0
R1       EQU   1
R2       EQU   2
R3       EQU   3
R4       EQU   4
R5       EQU   5
R6       EQU   6
R7       EQU   7
R8       EQU   8
R9       EQU   9
R10      EQU   10
R11      EQU   11
R12      EQU   12
R13      EQU   13
R14      EQU   14
R15      EQU   15

         END

When I run it, GPRs are:

DATA AT PSW  19800FBC - C0600004  00000000  01010A00
GR 0: 416487ED_5110B461   1: 40487ED5_110B4612

and we can't see the integer part in R1 .

(That final '..4612' rather than '..4611' is showing us that the rounding mode default is really 000 (".. with ties to even"), just as you said. When I originally reported the values I was unable to guarantee what default rounding mode was effective because the asm routine was being called from a high level language main program.)

But now comes the interesting part!

If you change the contents of the field FROMTHIS to '6.283185307179586476925286766559' (i.e. dropping the final 3 digits = 002) to have only 31 source digits instead, and then rerun the pgm, you will then see:

DATA AT PSW  19800FBC - C0600004 00000000  01010A00
GR 0: 416487ED_5110B461   1: 416487ED_5110B461

and the integer part is back!

Same marvel thing occurs when you have 32 or 33 source digits...

So, my conclusion is that Hercules' PFPO routine is not handling results correctly when there is exactly 34 DFP source digits. My best hunch is an alignment issue. We saw the integer part not being shown possibly because it was only 1 digit. If there were 2 we might have seen just the last.

Hope this helps!

Thanks again for your time.

Regards,

Gilson

@SDL-Hercules-390 SDL-Hercules-390 deleted a comment from gilsonlasco Aug 4, 2021
@Fish-Git
Copy link
Member

Fish-Git commented Aug 4, 2021

But now comes the interesting part:

If you change the contents of the field FROMTHIS to '6.283185307179586476925286766559' (i.e. dropping the final 3 digits = 002) to have only 31 source digits instead, and then rerun the pgm, you will then see:

DATA AT PSW  19800FBC - C0600004 00000000  01010A00
GR 0: 416487ED_5110B461   1: 416487ED_5110B461

and the integer part is back!

Interesting!

My test program is currently using the hard-coded binary register values you posted in your original problem report:

DFPIN_F4 DC    0D'0',XL8'39FFD2B32D873E6E'
DFPIN_F6 DC    0D'0',XL8'A9DAAD5ABE6B6404'
HFPOUTOK DC    0D'0',XL8'416487ED5110B45F'  (expected)

This is because the assembler that I am using (SATK/ASMA) does not currently support floating-point for 'D' type constants. It does support Decimal Floating-Point 'D' constants (e.g. LD'6.283185307179586476925286766559002'), but it unfortunately does not currently support Long Hexadecimal Floating-Point 'D' type constants.(*)   :(

Can you tell me what the exact hexadecimal binary constant values your assembler generates for:

LD'6.283185307179586476925286766559002'
D'6.283185307179586476925286766559002'

LD'6.283185307179586476925286766559'
D'6.283185307179586476925286766559'

(i.e. what value should I code for my XL16'.....' and XL8'.....' statements that would be equivalent to the above constants?)

I'd like to try your simple test for myself, but I don't know what hexadecimal values I should code. Can you show me your assembler listing? Be sure to insert a PRINT DATA statement so we can see everything! Thanks.

So, my conclusion is that Hercules' PFPO routine is not handling results correctly when there is exactly 34 DFP source digits. My best hunch is an alignment issue. We saw the integer part not being shown possibly because it was only 1 digit. If there were 2 we might have seen just the last.

You're probably right. I will try to look into this problem again to see if maybe I can figure it out with the clues you have provided. But don't hold your breath! Given my virtually non-existent skills at Floating-Point, it's unlikely I will be able to make much headway. Nevertheless, I shall give it a try.

Hope this helps!

It does! Thanks!

Thanks again for your time.

You are very welcome.


(*) Harold?  (@s390guy)  Is there any chance of ASMA ever eventually providing such support?

@gilsonlasco
Copy link
Author

gilsonlasco commented Aug 4, 2021

Well, you'll not see in asm listings the xl16 constants, but here it comes (part of it) anyway...

000000                00000 00068     1 FISH     CSECT                                                                      
                                      2 FISH     AMODE 31                                                                   
                                      3 FISH     RMODE ANY                                                                  
000000 B240 00E0                      4          BAKR  R14,R0                                                               
000004 C0C0 FFFF FFFE       00000     5          LARL  R12,FISH                                                             
                 R:C  00000           6          USING FISH,R12                                                             
00000A 6840 C038            00038     8          LD    F4,FROMTHIS                                                          
00000E 6860 C040            00040     9          LD    F6,FROMTHIS+8                                                        
000012 5800 C02C            0002C    10          L     R0,PARM                                                              
000016 010A                          11          PFPO                                                                       
000018 6000 C060            00060    12          STD   F0,PFPOGAVE                                                          
00001C E300 C050 0004       00050    14          LG    R0,IWANT                                                             
000022 E310 C060 0004       00060    15          LG    R1,PFPOGAVE                                                          
                                     16 * ------------------------------------------------------------------- *             
                                     17 * --  NOW YOU HAVE IN R0 WHAT IS EXPECTED FROM PFPO                -- *             
                                     18 * --                  R1 WHAT PFPO DELIVERS                        -- *             
                                     19 * ------------------------------------------------------------------- *    
000028 0000                          20          DC    H'0'  NOW A S0C1 TO INSPECT GPRS 0-1                        
00002A 0000                                                                                                        
00002C 01010A00                      22 PARM     DC    A(X'01010A00')                                              
000030                               23          DS    0D                                                          
000030 C6D9D6D4E3C8C9E2              25          DC    CL8'FROMTHIS'                                               
000038 22008064ACCB61CF              26 FROMTHIS DC    LD'6.283185307179586476925286766559' NOW HERCULES WORKS     
000048 C9C5E7D7C5C3E340              27          DC    CL8'IEXPECT '                                               
000050 416487ED5110B461              28 IWANT    DC    D'6.283185307179586476925286766559002'                      
000058 D7C6D7D6C7C1E5C5              30          DC    CL8'PFPOGAVE'                                               
000060 0000000000000000              31 PFPOGAVE DC    D'0'                                                        
000068                               33          LTORG                                                             

 
Here is part of JESLOG produced. Note that R0 and R1 now have the same value:

JOB GOODFISH         STEP EXEC            TIME 154535   DATE 21216    ID = 000    CPUID = 00618CBC2964   PAGE 00000001
COMPLETION CODE      SYSTEM = 0C1      REASON CODE = 00000001                                                         
                                                                                                                      
PSW AT ENTRY TO ABEND   078D0000  99800FC2  ILC  02  INTC  0001                                                     
PSW LOAD MODULE             ADDRESS = 19800F98  OFFSET = 0000002A                                                     
NAME=FISH
 ...
     DATA AT PSW  19800FBC - C0600004  00000000  01010A00     
     GR 0: 416487ED_5110B461   1: 416487ED_5110B461           
        2: 00000000_00000040   3: 00000000_007D89D4           
        4: 00000000_007D89B0   5: 00000000_007F8588           
        6: 00000000_007C7FC8   7: 00000000_00FB5D00           
        8: 00000000_007FC780   9: 00000000_007D0CB0           
        A: 00000000_00000000   B: 00000000_00000000           
        C: 00000000_19800F98   D: 00000000_00006F60           
        E: 00000000_80FDC1F0   F: 00000000_99800F98           
   END OF SYMPTOM DUMP                                        
  IEF450I GOODFISH EXEC - ABEND=S0C1 U0000 REASON=00000001    
... 
...
... lots of lines suppressed  ;-)
...

 
and now I'm attaching the execution dump (part of it) produced by that S0C1 so you can see directly in storage what you need:

1JOB GOODFISH         STEP EXEC            TIME 154535   DATE 21216    ID = 000                          PAGE 00000047      
                                                                                                                            
 19800FA0 FFFE6840 C0386860 C0405800 C02C010A    6000C060 E300C050 0004E310 C0600004   *... {..-{ ..{...-.{-T.{&..T.{-..*   
 19800FC0 00000000 01010A00 C6D9D6D4 E3C8C9E2    22008064 ACCB61CF 9BAA76AB 56AF9AD9   *........FROMTHIS....../........R*   
 19800FE0 C9C5E7D7 C5C3E340 416487ED 5110B461    D7C6D7D6 C7C1E5C5 416487ED 5110B461   *IEXPECT ..g..../PFPOGAVE..g..../*   
 19801000 00000000 00000000 00000000 00000000    00000000 00000000 00000000 00000000   *................................*   
       LINES 19801020-19801980  SAME AS ABOVE  

 
Regards,
Gilson

@Fish-Git
Copy link
Member

Fish-Git commented Aug 5, 2021

Thanks! I was also able, like you, to reproduce the correct behavior too, when using only 30 fractional digits.

I will try to step through the PFPO code paths for both cases (incorrect behavior vs. correct behavior) to see where the two diverge from one another. That should hopefully allow me to zero in on where the bug is.

(I just wish Bob's code was easier to follow!)   :(

@Fish-Git Fish-Git removed the QUESTION... A question was asked but has not been answered yet, -OR- additional feedback is requested. label Aug 5, 2021
@gilsonlasco
Copy link
Author

gilsonlasco commented Aug 5, 2021

Fine, glad to hear that.

BTW, I'm pretty sure that this alignment issue has something related to the return code in R1: this odd PFPO also sets a return code into R1 under certain (very obscure) conditions. PoP says that R1 contents is normally (most of the time...) zero, but for under overflow (underflow also?) R1 would have the exponent value, expressed as a 32-bit integer. That 34-digit issue is being possibly (wrongly) treated as an overflow condition. Well, truncation for integers are in leftmost digits, but in the rightmost ones for fractions, meaning loss of precision. What I see is PFPO treating decimals like integers and truncating to the left. Am I wrong?

@Fish-Git
Copy link
Member

Fish-Git commented Aug 5, 2021

That 34-digit issue is being possibly (wrongly) treated as an overflow condition. Well, truncation for integers are in leftmost digits, but in the rightmost ones for fractions, meaning loss of precision. What I see is PFPO treating decimals like integers and truncating to the left. Am I wrong?

I have no idea. As I said before, Floating-Point is largely beyond my feeble brain's ability to comprehend. When I try reading about it to try and understand it, my eyes go crossed and I start to get a headache.

@s390guy
Copy link
Contributor

s390guy commented Aug 8, 2021

Yes, there is hope of getting binary and hexadecimal floating point support in ASMA sometime.

I worked closely with Steve Orso when he was developing the floating point support we have in Hercules (but not PFPO). When it gets to testing it is the bit patterns that matter. ASMA's own generation of floating point values would only complicate the debugging process in this case. That is one of the lessons Steve and I learned about floating point. While standards exist for the actual computation of floating point values, none exist for the actual translation of text strings to/from floating point in any of the formats.

So, you are on the right track with the hex values.

Steve also spent a great deal of time dealing with things like scaling and rounding. I was not close to those efforts but your testing with FPC values is definitely also the right track. I am not aware of how R0 plays into PFPO's operation.

Identification of the "bug" seems to be a challenge due to a lack of details surrounding the exact environment in which PFPO is being used. This must be done to ever fix whatever could be at issue with PFPO.

@Fish-Git Fish-Git removed the IN PROGRESS... I'm working on it! (Or someone else is!) label Aug 9, 2021
@Fish-Git Fish-Git removed their assignment Aug 9, 2021
@Fish-Git
Copy link
Member

I worked closely with Steve Orso when he was developing the floating point support we have in Hercules (but not PFPO).

FYI: Steve wrote our Binary Floating Point support, but our PFPO support was written by Bob Wood, not Steve Orso.

@salva-rczero
Copy link
Contributor

salva-rczero commented May 7, 2022

Hi guys.

I was playing with float conversion in Rexx when google brought me to this page. After reading this thread about the PFPO bug, I decided to download the code and try to debug it. I am not an expert in Hercules, nor in mathematics, but my appreciation for your work pushed me to try.

And I found that the author of pfpo.c thought on lmd (I suppose lmd = "Left Most Digit"?) at:

   lmd = lmd * 1000;

hyperion/pfpo.c

Lines 1009 to 1018 in d0ccfbc

if (nan)
{
exp = 0;
lmd = 0;
}
else
{
exp = dflexp(expword, &lmd, dflwords);
lmd = lmd * 1000;
}

but forgot it in the Densely Packed Decimal add loop:

hyperion/pfpo.c

Lines 1049 to 1053 in d0ccfbc

for (i = 0;i < ndpd;i++)
{
arraydiv(dec,1024,ARRAYMAX,&rem);
decnum = DPD2BIN[rem];
memset(wrk, 0x00, ARRAYMAX * 4);

I added:
   if (i == ndpd - 1) decnum += lmd;
after:
   decnum = DPD2BIN[rem];

/***************************************************************/
/*   handle each set of 10 bits that represent a densely       */
/*   packed decimal number from 0-999.  Convert from densely   */
/*   packed decimal to normal, then multiply by 10**n, where   */
/*   n starts at zero and increases by three for each pass.    */
/*   Then take the result and add it to what we have so far.   */
/***************************************************************/
    for (i = 0;i < ndpd;i++)
    {
      arraydiv(dec,1024,ARRAYMAX,&rem);
      decnum = DPD2BIN[rem];
      if (i == ndpd - 1) decnum += lmd;
      memset(wrk, 0x00, ARRAYMAX * 4);

and it works!

Although I suppose it will require further testing.

Regards, salva.

@Fish-Git
Copy link
Member

Fish-Git commented May 7, 2022

Thank you Salva.   I will take a look at it and get back to you.

Bob? (@rwoodpd)     Steve? (@srorso)     Any comment?

Fish-Git pushed a commit that referenced this issue May 9, 2022
@Fish-Git
Copy link
Member

Fish-Git commented May 9, 2022

I added:
   if (i == ndpd - 1) decnum += lmd;
after:
   decnum = DPD2BIN[rem];
and it works!

Confirmed! (?)

Although...  I'm getting 416487ED 5110B461 instead of the 416487ED 5110B45F value that Gilson originally reported should be the correct result, but I'm presuming that's simply due to the rounding issue that was mentioned earlier in this thread?

Although I suppose it will require further testing.

Agreed.

Unfortunately though, I myself am not only unable to do so, but am also unqualified to do so, so we'll have to rely on someone else such as Gilson (@gilsonlasco) for that.

Once Gilson confirms from his point of view that the fix appears to be good, I will then go ahead and close this issue. (The fix for it has already been committed since I think it's good, but Gilson still needs to confirm it first.)

Thanks again, Salva! Good work!

@Fish-Git Fish-Git added the Waiting to close... Waiting for user to report back whether problem still exists or not before closing as resolved. label May 9, 2022
@salva-rczero
Copy link
Contributor

salva-rczero commented May 9, 2022

David, I also obtains 416487ED 5110B461 from my Rexx conversion routines, from PFPO in a real z15 & from HLASM. So I think it was a typo from @gilsonlasco.

It has been a great pleasure to contribute to this great project, even if it was just a drop. Thank you so much.

@Fish-Git
Copy link
Member

Fish-Git commented May 9, 2022

David, I also obtains 416487ED 5110B461 from my Rexx conversion routines, from PFPO in a real z15 & from HLASM. So I think it was a typo from @gilsonlasco.

Well, if a real z15 produces the same results, then I think we can trust that your fix is good!

The only question however is whether or not other values also produce correct results or not. We still don't have a formal test suite for the PFPO instruction yet after all. I wish we did, but we don't, so I think Gilson should have the final say on it.

gilsonlasco closed this 22 minutes ago

gilsonlasco reopened this 22 minutes ago

Hi Gilson! Is there a reason for your having closed and then re-opened this issue again? Does this mean your initial testing confirmed Salva's fix appears to be good, but that you maybe wanted more time to perform additional more detailed tests before considering it as having actually been resolved? Just curious!

@gilsonlasco
Copy link
Author

gilsonlasco commented May 9, 2022

Hi, Fish and salva-rczero.

Sorry for the delayed answer, I have to reestablish my testing environment to check it again.

The close was "accidental" (too many fingers in the hand...). It seems to be good, as the original complaint was for the highest part of the fraction ("Round to nearest with ties to even").

@gilsonlasco
Copy link
Author

gilsonlasco commented May 9, 2022

Again, David, from your storage dump:

HHC02290I R:0000000000000300  416487ED 5110B45F 00000000 00000000  ..g....^........
HHC02290I R:0000000000000310  40487ED5 110B4612 00000000 00000000   .=N............

the wrong value is "40487ED5... The correct is "416487ED...", achieved whenever input is less than 34 fractional digits in decfloat source (with or without the patch). I'll proceed with the tests, but, AFAIK, it is ok.

@Fish-Git
Copy link
Member

Fish-Git commented May 9, 2022

I'll proceed with the tests, but, AFAIK, it is ok.

Great! I'll keep this issue open until you finish your testing. Please let us know when you have done so. Thanks.

@gilsonlasco
Copy link
Author

The PMPO operation I've coded was to convert from 16byte extended decimal floating point to long hexadecimal floating point (8bytes). So, some precision loss is expected. But not to the left ;-)

Just to make it clearer, the value "416487ED5110B45F" was obtained from 31 source digits 6.283185307179586476925286766559) in decfloat format, and it is correct.

When I fed PMPO with 34 source digits (6.283185307179586476925286766559002) the correct value to be returned should be "416487ED5110B461" but hyperion, before the fix, wrongly returned "40487ED5110B4612".
After the fix, the value is correctly returned. So, it works! Congrats.

@Fish-Git
Copy link
Member

Thank you, Gilson. Closing Issue.

Fish-Git added a commit that referenced this issue May 26, 2022
@Fish-Git Fish-Git removed HELP! Help is needed from someone more experienced or I'm simply overloaded with too much work right now! Waiting to close... Waiting for user to report back whether problem still exists or not before closing as resolved. labels May 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BUG The issue describes likely incorrect product functionality that likely needs corrected.
Projects
None yet
Development

No branches or pull requests

6 participants