issue-#124 Range-check for SHORT(ENTIER(real))

The features that we decide to not apply in the current time
User avatar
Robert
Posts: 1024
Joined: Sat Sep 28, 2013 11:04 am
Location: Edinburgh, Scotland

Re: issue-#124 Range-check for SHORT(ENTIER(real))

Post by Robert »

Josef Templ wrote:I cannot find any docu where the compiler options are defined.
Is there any?
Not that I am aware of.

The text above is only based on my personal observations of what happens - I have not attempted to reverse engineer the actual compiler. These observations are probably incomplete, and may well contain errors, which I why I am keen to have comments.
luowy
Posts: 234
Joined: Mon Oct 20, 2014 12:52 pm

Re: issue-#124 Range-check for SHORT(ENTIER(real))

Post by luowy »

Ivan Denisov wrote:I found that this fix is leading to hight increase of drawing speed.

There is an stack overflow trap in Fern example. And with a fix it appears much faster!
fern.png
thanks Ivan, a backend fixup added,the whole fixup at below:

DevCPB.Convert

Code: Select all

	PROCEDURE Convert(VAR x: DevCPT.Node; typ: DevCPT.Struct);
		VAR node: DevCPT.Node; f, g: SHORTINT;
	BEGIN f := x.typ.form; g := typ.form;
		IF x.class = Nconst THEN
			IF g = String8 THEN
				IF f = String16 THEN String16ToString8(x)
				ELSIF f IN charSet THEN CharToString8(x)
				ELSE typ := DevCPT.undftyp
				END
			ELSIF g = String16 THEN
				IF f = String8 THEN String8ToString16(x)
				ELSIF f IN charSet THEN CharToString16(x)
				ELSE typ := DevCPT.undftyp
				END
			ELSE ConvConst(x.conval, f, g)
			END;
			x.obj := NIL
		ELSIF (x.class = Nmop) & (x.subcl = conv) & (DevCPT.Includes(f, x.left.typ.form) OR DevCPT.Includes(f, g)) 
		THEN
			(* don't create new node *)
			IF x.left.typ.form = typ.form THEN (* and suppress existing node *) x := x.left 
			ELSIF (f = Int64)&(g=Int32)&(x.left.typ.form =Real64)THEN (* SHORT(ENTIER(real)) *)
				node := DevCPT.NewNode(Nmop); node.subcl := conv; node.left := x; x := node;
			END
		ELSE
			IF (x.class = Ndop) & (x.typ.form IN {String8, String16}) THEN	(* propagate to leaf nodes *)
				Convert(x.left, typ); Convert(x.right, typ)
			ELSE
				node := DevCPT.NewNode(Nmop); node.subcl := conv; node.left := x; x := node;
			END
		END;
		x.typ := typ
	END Convert;
DevCPC486.ConvMove

Code: Select all

			ELSE
				(*y.form := f;*)
				IF m = Stk THEN
					y.form := f;
					IF ((f < Int32) OR (f = Char16)) & (y.mode # Reg) THEN LoadW(y, hint, stop) END;
					Push(y)
				ELSIF m # Undef THEN
					y.form := f;
					IF f = Int64 THEN
						IF y.mode # Reg THEN LoadLong(y, hint, stop) END;
						Free(y); y.form := Int32; z := x; z.form := Int32; DevCPL486.GenMove(y, z);
						IF z.mode = Reg THEN ASSERT(z.reg # y.index); z.reg := z.index ELSE INC(z.offset, 4) END;
						y.reg := y.index; DevCPL486.GenMove(y, z);
					ELSE
						IF y.mode # Reg THEN LoadW(y, hint, stop) END;
						Free(y); DevCPL486.GenMove(y, x)
					END
				ELSIF (y.mode = Stk)&(y.form = Int64)& (f = Int32 )THEN  (*SHORT(ENTIER(real))*)
					LoadLong(y, hint, stop);
					y.form := f
				ELSE
					 y.form := f
				END
			END
		END	
	END ConvMove;
I have tested the ObxFern module,no bug found;
luowy
Posts: 234
Joined: Mon Oct 20, 2014 12:52 pm

Re: issue-#124 Range-check for SHORT(ENTIER(real))

Post by luowy »

Josef Templ wrote:The current behavior MUST NOT be changed at all!
Removing an existing runtime check is a severe change that can affect existing code in a subtle way.
This is not acceptable for a last-minute change.
It does not lead to more robust code but to less robust code.
It also may add additional instructions and thus is potentially slower than the existing solution.
It also takes away the fine grained control over the range checking behavior.
With the current solution it is possible to express both cases, with and without range check.
This is not possible any longer with the change unless you add explicit cumbersome range checking code.

As a compromise I can imagine to add a note about range checking in the language report
or in the system specific part.
This is in line with Robert's proposal.

- Josef
I dont agree;
Removing an existing runtime check is a severe change that can affect existing code in a subtle way.
it do not affect existing code at all, all existing code have already add explicit cumbersome range checking for no trap.
It also may add additional instructions and thus is potentially slower than the existing solution.
it add only one instruction,how can it slower than the existing solution?
It also takes away the fine grained control over the range checking behavior.
With the current solution it is possible to express both cases, with and without range check.
This is not possible any longer with the change unless you add explicit cumbersome range checking code.
how can do remove range check? I want to know.
As a compromise I can imagine to add a note about range checking in the language report
or in the system specific part.
This is in line with Robert's proposal.
only document ENTIER is good enough.
Ivan Denisov
Posts: 1700
Joined: Tue Sep 17, 2013 12:21 am
Location: Russia

Re: issue-#124 Range-check for SHORT(ENTIER(real))

Post by Ivan Denisov »

luowy wrote:whole fixup at below:
After this fix there is an error during compilation of BlackBox.
DevComInterfaceGen.InitDialog
DevCom.png
DevCom.png (6.39 KiB) Viewed 82282 times
luowy
Posts: 234
Joined: Mon Oct 20, 2014 12:52 pm

Re: issue-#124 Range-check for SHORT(ENTIER(real))

Post by luowy »

need free registers,DevCPC486.ConvMove

Code: Select all

			ELSE
				(*y.form := f;*)
				IF m = Stk THEN
					y.form := f;
					IF ((f < Int32) OR (f = Char16)) & (y.mode # Reg) THEN LoadW(y, hint, stop) END;
					Push(y)
				ELSIF m # Undef THEN
					y.form := f;
					IF f = Int64 THEN
						IF y.mode # Reg THEN LoadLong(y, hint, stop) END;
						Free(y); y.form := Int32; z := x; z.form := Int32; DevCPL486.GenMove(y, z);
						IF z.mode = Reg THEN ASSERT(z.reg # y.index); z.reg := z.index ELSE INC(z.offset, 4) END;
						y.reg := y.index; DevCPL486.GenMove(y, z);
					ELSE
						IF y.mode # Reg THEN LoadW(y, hint, stop) END;
						Free(y); DevCPL486.GenMove(y, x)
					END
				ELSIF (y.mode = Stk)&(y.form = Int64)& (f = Int32 )THEN  (*SHORT(ENTIER(real))*)
					LoadLong(y, hint, stop);Free(y); (*free registers*)
					y.form := f
				ELSE
					 y.form := f
				END
			END
		END	
	END ConvMove;
User avatar
Josef Templ
Posts: 2047
Joined: Tue Sep 17, 2013 6:50 am

Re: issue-#124 Range-check for SHORT(ENTIER(real))

Post by Josef Templ »

Here is my proposal for adding docu for the CP compiler options, the default range checks and an update of Kernel.val.

http://redmine.blackboxframework.org/pr ... 657fa2fa71

It is based on Robert's draft with some extensions.

- Josef
User avatar
Josef Templ
Posts: 2047
Joined: Tue Sep 17, 2013 6:50 am

Re: issue-#124 Range-check for SHORT(ENTIER(real))

Post by Josef Templ »

luowy wrote: how can do remove range check? I want to know.
By assigning to LONGINT first, as Robert showed us.

In general, you want to have as many compile-time checks as possible.
For the remaining cases you want to have as many run-time checks as possible.
Only if you have a good reason for not checking something, you omit them.

For INTEGER arithmetics there are good reasons:
They need extra code, they don't allow us to write efficient random number generators
or secure hashing, crypto algorithms, etc., and people are used to MOD 2^32 two's complement arithmetic.

Conversion from REAL to integer does not fall into this category and the check comes for free.
If you think that the code is more robust by eliminating run-time checks then we should remove all of them.
The checked SHORT(ENTIER) is not a problem for anybody, only a surprising behavior.
The surprise can be eliminated easily by adding documentation.

- Josef
User avatar
Robert
Posts: 1024
Joined: Sat Sep 28, 2013 11:04 am
Location: Edinburgh, Scotland

Re: issue-#124 Range-check for SHORT(ENTIER(real))

Post by Robert »

Josef Templ wrote:Here is my proposal ...
While my mind is slightly open, I am not currently persuaded that changing the compiler for ENTIER is a good option.

I like Josef's proposal, but with a couple of very minor questions:

- Should we add that SHORT (REAL) leads to zero in the case of underflow? Does it, I don't know?
s is treated as a bit string (ARRAY of SET) and the resulting operation is e MOD 32 IN s[e DIV 32].
- I don't understand. Is it clearer to say
s is treated as an ARRAY of SET and the resulting operation is e MOD 32 IN s[e DIV 32], so out-of-range values for e attempt to access memory outside the actual memory allocated for s.
Ivan Denisov
Posts: 1700
Joined: Tue Sep 17, 2013 12:21 am
Location: Russia

Re: issue-#124 Range-check for SHORT(ENTIER(real))

Post by Ivan Denisov »

luowy wrote:need free registers,DevCPC486.ConvMove
This is leading to redefining of many negative constants... and the Kernel compilation error.

I agree with Josef, that we can not change compiler in the release candidate stage.
Ivan Denisov
Posts: 1700
Joined: Tue Sep 17, 2013 12:21 am
Location: Russia

Re: issue-#124 Range-check for SHORT(ENTIER(real))

Post by Ivan Denisov »

Josef Templ wrote:Here is my proposal for adding docu for the CP compiler options, the default range checks and an update of Kernel.val.

http://redmine.blackboxframework.org/pr ... 657fa2fa71

It is based on Robert's draft with some extensions.
Thanks for "Component Pascal compiler options" section!
Post Reply