From: Jan M. <jmo...@te...> - 2014-04-21 20:49:19
|
Hi, I found the following regression when compiling a version of the clem system (code as zip archive at [1]) with current master (complete log at [2]): in src/typed-ops/defmatrix-scale.lisp: ; caught WARNING: ; Derived type of SB-C::X is ; (VALUES RATIONAL &OPTIONAL), ; conflicting with its asserted type ; COMPLEX. ; See also: ; The SBCL Manual, Node "Handling of Types" So far I know that d299ab6 is good and 2ea1111 is bad. I will start to bisect now. Kind regards, Jan [1] https://ci.cor-lab.org/job/sbcl-master-test-grid/label=ubuntu_quantal_32bit/ws/cl-test-grid/work-dir/agent/quicklisp/dists/quicklisp/software/clem-20110418-git/*zip*/clem-20110418-git.zip [2] http://cl-test-grid.appspot.com/blob?key=1cc449uaf4 |
From: Cyrus H. <ch...@bo...> - 2014-04-21 23:06:00
|
I think clem is doing something (just one? :) ) fishy here. It should be declaring the type as the upgraded-array-element-type of the element type. There’s a fixed version here: https://github.com/slyrus/clem thanks, Cyrus On Apr 21, 2014, at 1:49 PM, Jan Moringen <jmo...@te...> wrote: > Hi, > > I found the following regression when compiling a version of the clem > system (code as zip archive at [1]) with current master (complete log at > [2]): > > in src/typed-ops/defmatrix-scale.lisp: > > ; caught WARNING: > ; Derived type of SB-C::X is > ; (VALUES RATIONAL &OPTIONAL), > ; conflicting with its asserted type > ; COMPLEX. > ; See also: > ; The SBCL Manual, Node "Handling of Types" > > So far I know that d299ab6 is good and 2ea1111 is bad. I will start to > bisect now. > > Kind regards, > Jan > > [1] https://ci.cor-lab.org/job/sbcl-master-test-grid/label=ubuntu_quantal_32bit/ws/cl-test-grid/work-dir/agent/quicklisp/dists/quicklisp/software/clem-20110418-git/*zip*/clem-20110418-git.zip > [2] http://cl-test-grid.appspot.com/blob?key=1cc449uaf4 > > > ------------------------------------------------------------------------------ > Start Your Social Network Today - Download eXo Platform > Build your Enterprise Intranet with eXo Platform Software > Java Based Open Source Intranet - Social, Extensible, Cloud Ready > Get Started Now And Turn Your Intranet Into A Collaboration Platform > http://p.sf.net/sfu/ExoPlatform > _______________________________________________ > Sbcl-devel mailing list > Sbc...@li... > https://lists.sourceforge.net/lists/listinfo/sbcl-devel |
From: Jan M. <jmo...@te...> - 2014-04-22 00:11:56
|
On Mon, 2014-04-21 at 22:49 +0200, Jan Moringen wrote: > I found the following regression when compiling a version of the clem > system (code as zip archive at [1]) with current master (complete log at > [2]): > > in src/typed-ops/defmatrix-scale.lisp: > > ; caught WARNING: > ; Derived type of SB-C::X is > ; (VALUES RATIONAL &OPTIONAL), > ; conflicting with its asserted type > ; COMPLEX. > ; See also: > ; The SBCL Manual, Node "Handling of Types" > > So far I know that d299ab6 is good and 2ea1111 is bad. I will start to > bisect now. The changed behavior seems to start with aab5bd0d43016a52b5cf0df34885f0d97b03a804 bad Author: Stas Boukarev <sta...@gm...> Date: Sat Apr 19 15:15:05 2014 +0400 Optimize (coerce x 'complex). Transform it into appropriate calls to COMPLEX, coercing to proper float types as necessary. Closes lp#1309815. Reduced test case: (defun a (q) (let ((qconv (coerce q 'complex))) (declare (type complex qconv)) qconv)) The problem is that (coerce RATIONAL 'complex) is of type RATIONAL. So this is an error in user-code as Cyrus Harmon already pointed out. However, a different problem can be illustrated with (defun a (q) (coerce q '(complex rational))) This causes NUMERIC-TYPE-FORMAT to return NIL for which the COERCE transform is not prepared, generating (COND (T (THE (COMPLEX RATIONAL) (COND ((NOT (TYPEP SB-C::X 'COMPLEX)) (COMPLEX (COERCE SB-C::X 'NIL))) ((TYPEP SB-C::X '(COMPLEX RATIONAL)) SB-C::X) (T (COMPLEX (COERCE (REALPART SB-C::X) 'NIL) (COERCE (IMAGPART SB-C::X) 'NIL))))))) Kind regards, Jan |
From: Jan M. <jmo...@te...> - 2014-04-22 01:55:10
Attachments:
0001-temp.patch
|
On Tue, 2014-04-22 at 02:11 +0200, Jan Moringen wrote: > On Mon, 2014-04-21 at 22:49 +0200, Jan Moringen wrote: > > However, a different problem can be illustrated with > > (defun a (q) > (coerce q '(complex rational))) > > This causes NUMERIC-TYPE-FORMAT to return NIL for which the COERCE > transform is not prepared, generating A sketch of a fix and a corresponding test is attached. What do you think? Kind regards, Jan |
From: Stas B. <sta...@gm...> - 2014-04-22 08:41:43
|
Jan Moringen <jmo...@te...> writes: > On Tue, 2014-04-22 at 02:11 +0200, Jan Moringen wrote: >> On Mon, 2014-04-21 at 22:49 +0200, Jan Moringen wrote: >> >> However, a different problem can be illustrated with >> >> (defun a (q) >> (coerce q '(complex rational))) >> >> This causes NUMERIC-TYPE-FORMAT to return NIL for which the COERCE >> transform is not prepared, generating > > A sketch of a fix and a corresponding test is attached. What do you > think? Looks alright. Will you commit it? -- With best regards, Stas. |
From: Jan M. <jmo...@te...> - 2014-04-22 14:13:32
|
On 04/22/2014 10:41 AM, Stas Boukarev wrote: > Jan Moringen <jmo...@te...> writes: > >> On Tue, 2014-04-22 at 02:11 +0200, Jan Moringen wrote: >>> On Mon, 2014-04-21 at 22:49 +0200, Jan Moringen wrote: >>> >>> However, a different problem can be illustrated with >>> >>> (defun a (q) >>> (coerce q '(complex rational))) >>> >>> This causes NUMERIC-TYPE-FORMAT to return NIL for which the COERCE >>> transform is not prepared, generating >> A sketch of a fix and a corresponding test is attached. What do you >> think? > Looks alright. Will you commit it? Probably later today. I would like to improve the test a bit. Kind regards, Jan |
From: Jan M. <jmo...@te...> - 2014-04-23 03:05:17
|
On Tue, 2014-04-22 at 12:41 +0400, Stas Boukarev wrote: > Jan Moringen <jmo...@te...> writes: > > > On Tue, 2014-04-22 at 02:11 +0200, Jan Moringen wrote: > >> On Mon, 2014-04-21 at 22:49 +0200, Jan Moringen wrote: > >> > >> However, a different problem can be illustrated with > >> > >> (defun a (q) > >> (coerce q '(complex rational))) > >> > >> This causes NUMERIC-TYPE-FORMAT to return NIL for which the COERCE > >> transform is not prepared, generating > > > > A sketch of a fix and a corresponding test is attached. What do you > > think? > Looks alright. Will you commit it? Unfortunately, improving the test produced the usual result: it revealed more errors :( As far as I can tell, the attached patch would fix all regressions introduced by the COERCE … COMPLEX source transform. That said, I'm not sure about (coerce 1/2 '(complex (real 1)) => 1/2 ; i.e. no error is signaled and similar cases. This seems quite strange but may actually be correct. The revealed errors are in the unoptimized version of COERCE (see disabled part of the new test) and are not regressions. Since we are in freeze, what do we do? Kind regards, Jan |
From: Stas B. <sta...@gm...> - 2014-04-27 12:16:21
|
Jan Moringen <jmo...@te...> writes: > On Tue, 2014-04-22 at 12:41 +0400, Stas Boukarev wrote: >> Jan Moringen <jmo...@te...> writes: >> >> > On Tue, 2014-04-22 at 02:11 +0200, Jan Moringen wrote: >> >> On Mon, 2014-04-21 at 22:49 +0200, Jan Moringen wrote: >> >> >> >> However, a different problem can be illustrated with >> >> >> >> (defun a (q) >> >> (coerce q '(complex rational))) >> >> >> >> This causes NUMERIC-TYPE-FORMAT to return NIL for which the COERCE >> >> transform is not prepared, generating >> > >> > A sketch of a fix and a corresponding test is attached. What do you >> > think? >> Looks alright. Will you commit it? > > Unfortunately, improving the test produced the usual result: it revealed > more errors :( > > As far as I can tell, the attached patch would fix all regressions > introduced by the COERCE … COMPLEX source transform. > > That said, I'm not sure about > > (coerce 1/2 '(complex (real 1)) => 1/2 ; i.e. no error is signaled > > and similar cases. This seems quite strange but may actually be correct. > > The revealed errors are in the unoptimized version of COERCE (see > disabled part of the new test) and are not regressions. Since we are in > freeze, what do we do? I simplified the patch a bit and committed it. We can deal with the non-conformance later. -- With best regards, Stas. |
From: Christophe R. <cs...@ca...> - 2014-04-23 06:56:27
|
Jan Moringen <jmo...@te...> writes: > On Tue, 2014-04-22 at 12:41 +0400, Stas Boukarev wrote: >> Looks alright. Will you commit it? > > Unfortunately, improving the test produced the usual result: it revealed > more errors :( > > As far as I can tell, the attached patch would fix all regressions > introduced by the COERCE … COMPLEX source transform. > > That said, I'm not sure about > > (coerce 1/2 '(complex (real 1)) => 1/2 ; i.e. no error is signaled > > and similar cases. This seems quite strange but may actually be correct. I think that's correct. The type (complex (real 1)) means all complexes, secretly; then the COERCE is explicitly allowed not to return a complex for rational arguments. (There isn't a similar distinction between specified and upgraded complex part types as there is with arrays). > The revealed errors are in the unoptimized version of COERCE (see > disabled part of the new test) and are not regressions. Since we are in > freeze, what do we do? Whoops. I think I'll let this through the freeze if Stas (or someone else) reviews it carefully; it seems low-risk, and there's definitely a regression to fix. Cheers, Christophe |
From: Jan M. <jmo...@te...> - 2014-04-23 08:42:40
|
On Wed, 2014-04-23 at 07:56 +0100, Christophe Rhodes wrote: > Jan Moringen <jmo...@te...> writes: > > > On Tue, 2014-04-22 at 12:41 +0400, Stas Boukarev wrote: > >> Looks alright. Will you commit it? > > > > Unfortunately, improving the test produced the usual result: it revealed > > more errors :( > > > > As far as I can tell, the attached patch would fix all regressions > > introduced by the COERCE … COMPLEX source transform. > > > > That said, I'm not sure about > > > > (coerce 1/2 '(complex (real 1)) => 1/2 ; i.e. no error is signaled > > > > and similar cases. This seems quite strange but may actually be correct. > > I think that's correct. The type (complex (real 1)) means all > complexes, secretly; then the COERCE is explicitly allowed not to return > a complex for rational arguments. (There isn't a similar distinction > between specified and upgraded complex part types as there is with arrays). I wasn't aware of the first rule; In fact, I didn't even know about UPGRADED-COMPLEX-PART-TYPE. > > The revealed errors are in the unoptimized version of COERCE (see > > disabled part of the new test) and are not regressions. Since we are in > > freeze, what do we do? > > Whoops. I think I'll let this through the freeze if Stas (or someone > else) reviews it carefully; it seems low-risk, and there's definitely a > regression to fix. I think the previous paragraph already revealed problems with the patch. It should probably rewritten to make use of UPGRADED-COMPLEX-PART-TYPE. To provide some ground truth, could you have a look at the table in the test and comment on whether the expected results seem correct (I now suspect there should be fewer errors signaled by either version of COERCE)? Thanks in advance and kind regards, Jan |
From: Stas B. <sta...@gm...> - 2014-04-23 09:37:34
|
Jan Moringen <jmo...@te...> writes: > On Wed, 2014-04-23 at 07:56 +0100, Christophe Rhodes wrote: >> Jan Moringen <jmo...@te...> writes: >> >> > On Tue, 2014-04-22 at 12:41 +0400, Stas Boukarev wrote: >> >> Looks alright. Will you commit it? >> > >> > Unfortunately, improving the test produced the usual result: it revealed >> > more errors :( >> > >> > As far as I can tell, the attached patch would fix all regressions >> > introduced by the COERCE … COMPLEX source transform. >> > >> > That said, I'm not sure about >> > >> > (coerce 1/2 '(complex (real 1)) => 1/2 ; i.e. no error is signaled >> > >> > and similar cases. This seems quite strange but may actually be correct. >> >> I think that's correct. The type (complex (real 1)) means all >> complexes, secretly; then the COERCE is explicitly allowed not to return >> a complex for rational arguments. (There isn't a similar distinction >> between specified and upgraded complex part types as there is with arrays). > > I wasn't aware of the first rule; In fact, I didn't even know about > UPGRADED-COMPLEX-PART-TYPE. > >> > The revealed errors are in the unoptimized version of COERCE (see >> > disabled part of the new test) and are not regressions. Since we are in >> > freeze, what do we do? >> >> Whoops. I think I'll let this through the freeze if Stas (or someone >> else) reviews it carefully; it seems low-risk, and there's definitely a >> regression to fix. > > I think the previous paragraph already revealed problems with the patch. > It should probably rewritten to make use of UPGRADED-COMPLEX-PART-TYPE. > To provide some ground truth, could you have a look at the table in the > test and comment on whether the expected results seem correct (I now > suspect there should be fewer errors signaled by either version of > COERCE)? If u-c-p-t is to be used, then our whole implementation COMPLEX types is incorrect. "(complex type-specifier) refers to all complexes that can result from giving numbers of type type-specifier to the function complex, _plus all other complexes of the same specialized representation_" and "upgraded complex part type n. (of a type) a type that is a supertype of the type and that is used instead of the type whenever the type is used as a complex part type for object creation or type discrimination." by that notion, (typep #c(2 1) '(complex (eql 1))) should return T, but no implementation, except lispworks, does that. -- With best regards, Stas. |