Conflicting OPTIMIZE declarations within a same "declarations block" unconservatively favor the last declaration of its kind.

Bug #724020 reported by Jean-Philippe Paradis
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
SBCL
New
Undecided
Unassigned

Bug Description

What I do:
(defun optimize-speed? (a b)
  (declare (optimize (speed 0) (speed 3)))
  (+ a b))

(optimize-speed? 1 2)
=> 3

What happens:
Execution proceeds without an error of any kind being thrown. The result is also "technically correct" (the best kind of "correct"!). But what happened to my conflicting declarations?? Let's ask sb-ext:describe-compiler-policy:

(sb-ext:describe-compiler-policy '((speed 0) (speed 3)))
-| [...] SPEED = 3 [...]

(sb-ext:describe-compiler-policy '((speed 3) (speed 0)))
-| [...] SPEED = 0 [...]

So, apparently the last specification for each OPTIMIZE quality overrides the preceding ones. Which is exactly what I'd expect, for example, for:
(defun optimize-speed? (a b)
  (declare (optimize (speed 0)))
  (locally (declare (optimize (speed 3)))
    (+ a b)))

What I expected to happen:
I'll be the first to say that this behavior doesn't contradict the standard. From CLHS OPTIMIZE:

"The consequences are unspecified if a quality appears more than once with different values. "

But in the case where multiple conflicting OPTIMIZE declarations appear within a same "declarations block", as in the first example, I think the best behavior would be to signal an error at compile-time.

I think it's a very desirable property that most types of declarations within a same "declarations block" are "naturally" order-independent as a side-effect of their semantics. It's tragic to just barely lose this property for, as far as I can tell, no gain.

I also think such conflicting declarations are usually the result of programming errors (such as careless handling of declarations in macroexpansions). I can't really think of a scenario where someone would deliberately use this "feature", and also someone relying on such a feature would likely find a different behavior in other implementations. Maybe a quick test of how other implementations deal with this situation would be enlightening.

So, to sum up, I think conflicting OPTIMIZE declarations within a same "declarations block" should signal an error at compile-time. Else, the current behavior should be documented. I found no trace of this in the SBCL manual.

Here's a more complete test-case:

(defun optimize-speed? (a b)
  (declare (optimize (compilation-speed 0) (compilation-speed 3)
                     (debug 3) (debug 0)
                     (safety 0) (safety 3)
                     (space 3) (space 0)
                     (speed 0) (speed 3)))
  (+ a b))

SBCL version: 1.0.42
uname -a: Linux dynamorph 2.6.32-27-generic #49-Ubuntu SMP Wed Dec 1 23:52:12 UTC 2010 i686 GNU/Linux

*features*:
(:SWANK :QUICKLISP :SB-BSD-SOCKETS-ADDRINFO :ASDF2 :ASDF :ANSI-CL :COMMON-LISP
 :SBCL :SB-DOC :SB-TEST :SB-LDB :SB-PACKAGE-LOCKS :SB-UNICODE :SB-EVAL
 :SB-SOURCE-LOCATIONS :IEEE-FLOATING-POINT :X86 :UNIX :ELF :LINUX :SB-THREAD
 :LARGEFILE :GENCGC :STACK-GROWS-DOWNWARD-NOT-UPWARD :C-STACK-IS-CONTROL-STACK
 :COMPARE-AND-SWAP-VOPS :UNWIND-TO-FRAME-AND-CALL-VOP :RAW-INSTANCE-INIT-VOPS
 :STACK-ALLOCATABLE-CLOSURES :STACK-ALLOCATABLE-VECTORS
 :STACK-ALLOCATABLE-LISTS :STACK-ALLOCATABLE-FIXED-OBJECTS :ALIEN-CALLBACKS
 :CYCLE-COUNTER :INLINE-CONSTANTS :MEMORY-BARRIER-VOPS :LINKAGE-TABLE
 :OS-PROVIDES-DLOPEN :OS-PROVIDES-PUTWC :OS-PROVIDES-SUSECONDS-T)

Revision history for this message
Roman Marynchak (roman-marynchak) wrote :

This is mostly a duplicate of https://bugs.launchpad.net/sbcl/+bug/310267

Revision history for this message
Tobias C. Rittweiler (tcr) wrote :

For the briefly-glancing reader, please notice that Jean /correctly/ said
that

  (defun foo (x y)
    (declare (optimize (safety 0)))
    (locally (declare (optimize (safety 3))
      ...))

is DIFFERENT to

  (defun foo* (x y)
     (declare (optimize (safety 0))
     (declare (optimize (safety 3))
     ...)

The latter has unspecified consequences (multiple ambiguous
declarations given) whereas the former means that argument
processing and result passing should happen under (safety 0)
whereas the rest of the definition should happen under (safety 3).

Revision history for this message
Nikodemus Siivola (nikodemus) wrote :

There is a sense in which

 (declare (optimize (speed 3) (speed 0)))

is distinct from

 (declare (optimize (speed 3)))
 (declare (optimize (speed 0)))

but frankly, I'm not at all sure that supporting such a distinction is worth the trouble for anyone. My preference is to handle both cases identically. Specifically, I cannot see where in the spec the first case is given any more specified consequences than the second one.

Note that if you stick a LOCALLY in there to separate the two declarations, we're back in well specified land:

 (declare (optimize (speed 3)))
 (locally (declare (optimize (speed 0))) ...)

is perfectly fine, yes. The other ones are not -- CLHS doesn't specify the order in which declarations are processed, and given the semantics it seems most natural to me at least that they should take effect simultaneously, in which case conflicting parallel ones are either nonsensical or need to be merged somehow.

My current stance is that we should signal a STYLE-WARNING, and use the maximum specified level instead of sequential dominance so that (SAFETY 3) doesn't get accidentally trumped.

Revision history for this message
Jean-Philippe Paradis (hexstream) wrote :

I have two points to talk about:

1. Take it for what it's worth, but right now I'm working on a declarations processing library for which it would be highly undesirable that

(declare (optimize (speed 3) (speed 0)))

has different semantics than

(declare (optimize (speed 3)))
(declare (optimize (speed 0)))

, because among other things it will provide for customizable "normalization" of declarations, which can rearrange the merging, order, proximity and shape of declarations (or not) according to user preferences. This would let one provide more consistent and easily eye-scannable macroexpansions.

I'm not yet completely sure that such an idea is as useful as it sounds to me right now, but I know that if we start arbitrarily assigning semantics to the "shape" of declarations within a declarations block, this kind of processing will be arbitrarily crippled.

Also, such additional semantics would have a high chance of differing between implementations (portability nightmare), which is particularly alarming because the effect of declarations is oftentimes quite subtle and the user won't necessarily have any hint at all that his declarations didn't actually have the intended effects.

2. You said:

"My current stance is that we should signal a STYLE-WARNING, and use the maximum specified level instead of sequential dominance so that (SAFETY 3) doesn't get accidentally trumped."

I'll point out that "maximum specified level" is only really well-defined for SAFETY.

For instance, what's "maximum specified level" for SPEED? (SPEED 3), right? But in the presence of (SAFETY 3), wouldn't the "maximum specified level" of speed then be 0?? And the (SAFETY 3) might actually come from a (SAFETY 3) (SAFETY 0) according to these semantics! Hello insane declaration interactions!!

In conclusion, I'm of the opinion that we should steer clear of assigning ANY semantics to the shape of declarations, including merging, order, proximity and shape of declarations.

Here are examples of what I mean by these attributes to make things clear (I didn't try to come up with problematic examples, just illustrating the attributes):

Merging:
(declare (optimize (speed 3) (safety 0)))
VS
(declare (optimize (speed 3))) (declare (optimize (safety 0)))

Order:
(declare (optimize (speed 3) (safety 0)))
VS
(declare (optimize (safety 0) (speed 3)))

Proximity (similar to but different than Order. Here, it's no normalization VS one kind of proximity normalization):
(declare (optimize (speed 3)))
(declare (inline foo))
(declare (optimize (safety 0)))
VS
(declare (optimize (speed 3)))
(declare (optimize (safety 0)))
(declare (inline foo))

Shape:
(declare (optimize safety))
VS
(declare (optimize (safety 3)))

Here I only used OPTIMIZE declarations but this applies to all other declarations in general. And also, obviously, these attributes are orthogonal and their normalization strategies can be combined.

Revision history for this message
Jean-Philippe Paradis (hexstream) wrote :

Oh, and let's not forget that there are implementation-dependent and possibly even user-defined declarations. If we assign semantics to the shape of declarations, an implementation or user coming up with his own new types of declarations would have to choose between being highly conservative, sticking only to what is well-defined by the standard, or try to extend the "shape semantics" of special precedence rules or whatnot to his own declarations.

Inevitably, some implementations and users would choose the former, and others would choose the latter. And even among this last group, the choice of what set of semantics to build on may very well be different between implementations and users.

This is stacking arbitrary design decisions on top of other arbitrary design decisions, when restraint should have been exercised in the first place, skipping the whole unnecessary complications :( As a Ruby user would say, "This won't scale!"

And we really only analysed possible "shape semantics" with respect to OPTIMIZE declarations so far.

Revision history for this message
Nikodemus Siivola (nikodemus) wrote :

I fully confess I only skimmed your replies. I don't have enough cycles to burn on this right now to think them through, but:

* I strongly oppose assigning any implementation specific meaning to duplicate qualities in either form. I fail to see any benefit there, as it will only lead to porting issues. If you need scoping, use

 (lambda (...)
   (declare (optimize ...for lambda-list...))
   (locally (declare (optimize ...for body...))
     ...))

instead. "Shape" of declarations should not matter, only the lexical contour.

* No other policy is allowed to override the specified effects of SAFETY 3, so as long as SAFETY is merged using MAX, it doesn't really matter from correctness POV how others are merged.

* Scoping of declarations is perfectly clear. The only question is how to handle/merge multiple for the same thing in the same scope. For TYPE eg. the spec implies intersection of types. For user-defined declarations user code introspecting the environment is responsible for that -- and I submit that user-code tries to deduce the "shape" and derive semantics from that is doing something brittle and is liable to break. I really fail to see the problem here.

Not to be overly grumpy, but if you want a different resolution than the one I'm heading towards right now (STYLE-WARNING plus merge using MAX) please provide a short (preferably runnable example) that demonstrates the problems you see with this approach,

Revision history for this message
Nikodemus Siivola (nikodemus) wrote :

Ah, I see you got confused by my usage of "maximum specified level".

I meant that both

  (lambda (x)
    (declare (optimize (speed 1) (speed 3) (safety 0) (safety 2) (speed 2) (safety 1)))
    ...)

  (lambda (x)
    (declare (optimize (speed 3) (safety 1)))
    (declare (optimize (speed 0) (safety 2)))
    ...)

should signal a STYLE-WARNING, and be compiled as if

  (lambda (x)
    (declare (optimize (speed 3) (safety 2)))
    ...)

had been used instead, though as far as I'm concerned only making sure that happens for SAFETY really matters -- nothing else has required semantics.

Revision history for this message
Jean-Philippe Paradis (hexstream) wrote :

We're in violent agreement!

Not only do I completely concur with all your points, it's exactly what I was arguing in favor of.

Revision history for this message
Nikodemus Siivola (nikodemus) wrote :

Ok, good. :)

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.