search

vishapoberon / compiler

vishap oberon compiler
http://oberon.vishap.am
GNU General Public License v3.0
186 stars 25 forks source link

Fix for procedure Expo in module Reals #42

Open kekcleader opened 7 years ago

kekcleader commented 7 years ago

On my Debian 32 bit, procedure Reals.Expo (for REAL) didn't work right, whereas ExpoL (for LONGREAL) did work. As a result, Texts.WriteReal and Texts.WriteRealFix did not work (Texts.WriteLongReal did work). I went ahead and fixed it (in src/library/v4/Reals.Mod):

  PROCEDURE Expo*(x: REAL): INTEGER;
    VAR i: INTEGER; l: LONGINT;
  BEGIN
    IF SIZE(INTEGER) = 4 THEN
      S.GET(S.ADR(x), i); 
      i := SHORT(ASH(i, -23) MOD 256)
    ELSIF SIZE(LONGINT) = 4 THEN
      S.GET(S.ADR(x), l); 
      i := SHORT(ASH(l, -23) MOD 256)
    ELSE HALT(98)
    END;
    RETURN i
  END Expo;

Before that it was just this:

  PROCEDURE Expo*(x: REAL): INTEGER;
  BEGIN
    RETURN SHORT(ASH(S.VAL(INTEGER, x), -23) MOD 256)
  END Expo;

And probably the issue had something to do with the incorrect work of SYSTEM.VAL for REAL to INTEGER conversion.

P.S. Not sure if SIZE(INTEGER) = 4 is required, but I added it for the future. The optimization removes the IF statement during compilation anyway. I used the graphic over here for reference and this simple test program:

MODULE R;
IMPORT Out := Console, Reals, S := SYSTEM, Oberon, Texts;

PROCEDURE Expo(x: REAL): INTEGER;
  VAR i: INTEGER; l: LONGINT;
BEGIN
  IF SIZE(INTEGER) = 4 THEN
    S.GET(S.ADR(x), i);
    i := SHORT(ASH(i, -23) MOD 256)
  ELSIF SIZE(LONGINT) = 4 THEN
    S.GET(S.ADR(x), l);
    i := SHORT(ASH(l, -23) MOD 256)
  ELSE HALT(98)
  END;
  RETURN i
END Expo;

PROCEDURE Do;
VAR x: REAL; i: INTEGER; W: Texts.Writer;
BEGIN
  Texts.OpenWriter(W);
  x := 0.15625; (* Expo should return 124 = 01111100 *)
  REPEAT
    Texts.WriteLongReal(W, LONG(x), 25);
    Texts.Append(Oberon.Log, W.buf);
    i := Expo(x); Out.Int(i, 6); Out.Ln;
    x := x * 2
  UNTIL x > 10000
END Do;

BEGIN
  Do
END R.
dcwbrown commented 7 years ago

There are loads of problems with real handling because of invalid size assumptions in the code.

It happens by chance that I had already looked at this and committed the following solution to the master branch 5 days ago: 

 PROCEDURE Expo*(x: REAL): INTEGER;
 VAR i: INTEGER;
 BEGIN
 SYSTEM.GET(SYSTEM.ADR(x)+2, i);
 RETURN (i DIV 128) MOD 256
 END Expo;

Sorry - you must have just missed it :-(.

It's a simpler solution - would you mind trying it with your test?

There are so many issues with real across many, many library sources, due to varying type sizes. Indeed I recently disabled a load of library modules because they were not handling reals correctly.

I'm working on fixed integer types which will make coding the real handling easier. It'll take me a while I'm afraid. But my goal is to recode all the real implementations to be independent of SHORTINT/INTEGER/LONGINT size.

Thanks -- Dave.

On 2016-08-29 16:30, Артур Ефимов wrote:

On my Debian 32 bit, procedure Reals.Expo (for REAL) didn't work right, whereas ExpoL (for LONGREAL) did work. As a result, Texts.WriteReal and Texts.WriteRealFix did not work (Texts.WriteLongReal did work). I went ahead and fixed it (in src/library/v4/Reals.Mod):

PROCEDURE Expo*(x: REAL): INTEGER; VAR i: INTEGER; l: LONGINT; BEGIN IF SIZE(INTEGER) = 4 THEN S.GET(S.ADR(x), i); i := SHORT(ASH(i, -23) MOD 256) ELSIF SIZE(LONGINT) = 4 THEN S.GET(S.ADR(x), l); i := SHORT(ASH(l, -23) MOD 256) ELSE HALT(98) END; RETURN i END Expo;

Before that it was just this:

PROCEDURE Expo*(x: REAL): INTEGER; BEGIN RETURN SHORT(ASH(S.VAL(INTEGER, x), -23) MOD 256) END Expo;

And probably the issue had something to do with the incorrect work of SYSTEM.VAL for REAL to INTEGER conversion.

P.S. Not sure if SIZE(INTEGER) = 4 is required, but I added it for the future. The optimization removes the IF statement during compilation anyway. I used the graphic over here [1] for reference and this simple test program:

MODULE R; IMPORT Out := Console, Reals, S := SYSTEM, Oberon, Texts;

PROCEDURE Expo(x: REAL): INTEGER; VAR i: INTEGER; l: LONGINT; BEGIN IF SIZE(INTEGER) = 4 THEN S.GET(S.ADR(x), i); i := SHORT(ASH(i, -23) MOD 256) ELSIF SIZE(LONGINT) = 4 THEN S.GET(S.ADR(x), l); i := SHORT(ASH(l, -23) MOD 256) ELSE HALT(98) END; RETURN i END Expo;

PROCEDURE Do; VAR x: REAL; i: INTEGER; W: Texts.Writer; BEGIN Texts.OpenWriter(W); x := 0.15625; (* Expo should return 124 = 01111100 ) REPEAT Texts.WriteLongReal(W, LONG(x), 25); Texts.Append(Oberon.Log, W.buf); i := Expo(x); Out.Int(i, 6); Out.Ln; x := x \ 2 UNTIL x > 10000 END Do;

BEGIN Do END R.

You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub [2], or mute the thread [3].

Links:

[1] https://ru.wikipedia.org/wiki/%D0%A7%D0%B8%D1%81%D0%BB%D0%BE_%D0%BE%D0%B4%D0%B8%D0%BD%D0%B0%D1%80%D0%BD%D0%BE%D0%B9_%D1%82%D0%BE%D1%87%D0%BD%D0%BE%D1%81%D1%82%D0%B8 [2] https://github.com/vishaps/voc/issues/42 [3] https://github.com/notifications/unsubscribe-auth/ADChoDbohJmSwltnEcX6F-70SW3glcAkks5qkvstgaJpZM4Jvl96

kekcleader commented 7 years ago

Hi Dave,

Yes, I must have missed the update. I've checked your solution with my test and it works.

Sidenote: Just curious, what do you think about removing parentheses from around i DIV 128 to make it: RETURN i DIV 128 MOD 256 ? Does it look more complex or simplier to you than with the parentheses?

Arthur

2016-08-29 20:30 GMT+03:00 David C W Brown notifications@github.com:

There are loads of problems with real handling because of invalid size assumptions in the code.

It happens by chance that I had already looked at this and committed the following solution to the master branch 5 days ago:

PROCEDURE Expo*(x: REAL): INTEGER;
VAR i: INTEGER;
BEGIN
SYSTEM.GET(SYSTEM.ADR(x)+2, i);
RETURN (i DIV 128) MOD 256
END Expo;

Sorry - you must have just missed it :-(.

It's a simpler solution - would you mind trying it with your test?

There are so many issues with real across many, many library sources, due to varying type sizes. Indeed I recently disabled a load of library modules because they were not handling reals correctly.

I'm working on fixed integer types which will make coding the real handling easier. It'll take me a while I'm afraid. But my goal is to recode all the real implementations to be independent of SHORTINT/INTEGER/LONGINT size.

Thanks -- Dave.

On 2016-08-29 16:30, Артур Ефимов wrote:

On my Debian 32 bit, procedure Reals.Expo (for REAL) didn't work right, whereas ExpoL (for LONGREAL) did work. As a result, Texts.WriteReal and Texts.WriteRealFix did not work (Texts.WriteLongReal did work). I went ahead and fixed it (in src/library/v4/Reals.Mod):

PROCEDURE Expo*(x: REAL): INTEGER; VAR i: INTEGER; l: LONGINT; BEGIN IF SIZE(INTEGER) = 4 THEN S.GET(S.ADR(x), i); i := SHORT(ASH(i, -23) MOD 256) ELSIF SIZE(LONGINT) = 4 THEN S.GET(S.ADR(x), l); i := SHORT(ASH(l, -23) MOD 256) ELSE HALT(98) END; RETURN i END Expo;

Before that it was just this:

PROCEDURE Expo*(x: REAL): INTEGER; BEGIN RETURN SHORT(ASH(S.VAL(INTEGER, x), -23) MOD 256) END Expo;

And probably the issue had something to do with the incorrect work of SYSTEM.VAL for REAL to INTEGER conversion.

P.S. Not sure if SIZE(INTEGER) = 4 is required, but I added it for the future. The optimization removes the IF statement during compilation anyway. I used the graphic over here [1] for reference and this simple test program:

MODULE R; IMPORT Out := Console, Reals, S := SYSTEM, Oberon, Texts;

PROCEDURE Expo(x: REAL): INTEGER; VAR i: INTEGER; l: LONGINT; BEGIN IF SIZE(INTEGER) = 4 THEN S.GET(S.ADR(x), i); i := SHORT(ASH(i, -23) MOD 256) ELSIF SIZE(LONGINT) = 4 THEN S.GET(S.ADR(x), l); i := SHORT(ASH(l, -23) MOD 256) ELSE HALT(98) END; RETURN i END Expo;

PROCEDURE Do; VAR x: REAL; i: INTEGER; W: Texts.Writer; BEGIN Texts.OpenWriter(W); x := 0.15625; (* Expo should return 124 = 01111100 ) REPEAT Texts.WriteLongReal(W, LONG(x), 25); Texts.Append(Oberon.Log, W.buf); i := Expo(x); Out.Int(i, 6); Out.Ln; x := x \ 2 UNTIL x > 10000 END Do;

BEGIN Do END R.

You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub [2], or mute the thread [3].

Links:

[1] https://ru.wikipedia.org/wiki/%D0%A7%D0%B8%D1%81%D0%BB%D0% BE%D0%BE%D0%B4%D0%B8%D0%BD%D0%B0%D1%80%D0%BD%D0%BE%D0%B9 %D1%82%D0%BE%D1%87%D0%BD%D0%BE%D1%81%D1%82%D0%B8 [2] https://github.com/vishaps/voc/issues/42 [3] https://github.com/notifications/unsubscribe-auth/ADChoDbohJmSwltnEcX6F- 70SW3glcAkks5qkvstgaJpZM4Jvl96

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/vishaps/voc/issues/42#issuecomment-243193824, or mute the thread https://github.com/notifications/unsubscribe-auth/ACQZMmvcN7O3r0AP-pSM1YKAHplR6Naaks5qkxdKgaJpZM4Jvl96 .

dcwbrown commented 7 years ago

Good question. I feel more comfortable with the parentheses.

Using C/C++ I became accustomed to adding technically redundant parentheses to avoid finding yet again that C's rules had surprised me.

However I have just looked back to code I was writing in 1993 when I was happily using Modula-2 and yet to be adulterated by C, and found this:

CASE (hardware DIV 16) MOD 4 OF

So perhaps I shouldn't blame C.