Style warning cleanup #826
Conversation
…ing benchmarks This assignment must be outdated, because it binds a name `*cost-fn-weight-style*` which uses the special variable convention, but is not defined or declaimed special anywhere. The benchmarks do not use this assignment, so I believe it's safe to remove.
magicl-constructors.lisp has a compile-time dependency on ast.lisp because it uses some inline functions (struct accessors) defined in the latter file, but ast.lisp has no compile-time dependency on magicl-constructors.lisp. SBCL warned about the old order, but doesn't about the new order.
In order to inline a function, it must be declaimed `inline` before its definition, and its definition must appear before any of its uses. SBCL issues style-warnings if any of those conditions don't apply. This commit reorders some forms in clifford/stabilizer.lisp to allow inlining and make the warning go away.
…nd compilation-methods Prior to this commit, chip-specification.lisp globally declaimed the accessors on `hardware-object` as `notinline`, in the hopes that would shut SBCL up about its inability to inline the accessors. It didn't work. Now, each usage of one of those accessors before their definitions is locally declared `notinline`, which makes the warnings go away. The global `notinline` declaimations are removed, as they never accomplished anything other than preventing optimizations.
The keyword argument `:initial-rewiring` appeared in the arglist for the default method, and in the generic function's docstring, but it did not appear in the generic function's declared arglist (nor did `&allow-other-keys`), and the default method never read from `initial-rewiring`. References in the method arglist and the docstring are removed.
SBCL will warn when it compiles a call to a function for which an `inline` declaration is in effect, but for which inlining is impossible. It will also warn when it compiles the definition of a function for which an `inline` declaration is effect, but to which not-inlined calls have previously been compiled. Both of these cases appeared in define-compiler.lisp. CL-HEAP contains `inline` declaimations for `enqueue` and `dequeue`, but those are generic functions and therefore not elligible for inlining. Structure accessors are always `inline`, but define-compiler forward-references accessors to the `gate-record` struct. Rather than addressing the root of either of those problems, this commit adds local `notinline` declarations so SBCL will not warn.
No warning to fix here, but an opportunity for some minor missed compiler optimizations. CL compilers are generally unable to do any useful optimization or compile-time type checking with `satisfies` types, because they are incapable of understanding the behavior of the `satisfies` function. The traditional solution is, for a type FOO and a predicate function FOO-WITH-SOME-PROPERTY-P, to define the type FOO-WITH-SOME-PROPERTY as `(and FOO (satisfies FOO-WITH-SOME-PROPERTY-P))`, not just `(satisfies FOO-WITH-SOME-PROPERTY-P)`. That way, the compiler knows at compile time that every `FOO-WITH-SOME-PROPERTY` is at the very least a `FOO`, and also that any `(not FOO)` is also not a `FOO-WITH-SOME-PROPERTY`. Footgun: `FOO-WITH-SOME-PROPERTY-P` still needs to check `(typep thing 'FOO)`.
| @@ -240,10 +240,14 @@ The keys are typically INSTRUCTION instances and associated values are STRINGs." | |||
| (integer-vec | |||
| (map 'vector | |||
| (lambda (token) | |||
| (declare | |||
There was a problem hiding this comment.
can we just move this whole rewiring section to be defined after token is?
| hardware-object-rewriting-rules | ||
| hardware-object-cxns | ||
| hardware-object-misc-data)) | ||
| ;;; Note that a previous file "compilation-methods.lisp" uses this structure, but the dependency is circular, |
There was a problem hiding this comment.
Can we reverse it? Can we
(declaim inline)
(defstruct)
(declaim notinline)
and just inline whenever we feel we need to?
Adding bulky notinline decls everywhere just to quiet SBCL is sort of a code smell to me; I'd rather condense it all, and use inline to drive performance, not notinline to hush warnings.
| @@ -77,9 +77,16 @@ | |||
| (application-thread-invocation-thread instruction))) | |||
| ;; then try the compilers attached to the hardware object | |||
| (when hardware-object | |||
| (map nil #'try-compiler (hardware-object-compilation-methods hardware-object))) | |||
| (map nil #'try-compiler | |||
| (locally | |||
| (instr nil :type application) | ||
| (prev nil :type (or null peephole-rewriter-node)) | ||
| (next nil :type (or null peephole-rewriter-node)) | ||
| (context nil)) ; compressor context *after* this instruction is applied |
There was a problem hiding this comment.
this comment got lost in the copy-paste
| @@ -654,6 +661,10 @@ N.B.: The word \"shortest\" here is a bit fuzzy. In practice it typically means | |||
| (let ((queue (make-instance 'cl-heap:priority-queue :sort-fun (complement #'occurrence-table-metric-worsep)))) | |||
| (flet ((collect-bindings (occurrence-table) | |||
| (a:hash-table-keys (occurrence-table-map occurrence-table)))) | |||
| (declare | |||
There was a problem hiding this comment.
this notinline is totally fine.
Also can you M-q the comment?
What line length do you prefer?
My gut says no;
No, I don't believe this works. If it did, the previous version that just
I aimed for 4 because it seemed the least invasive fix, but 2 seems more correct. 1 feels like a gross hack, and 3 seems unlikely to work. |
|
If you just want SBCL to stop complaining about certain warnings, I think adding inline/notinline to shut the compiler up would be poor style. Moving definitions around works if you want speed, as the style warnings note. Maybe those warnings should get downgraded to just compiler notes... |
|
Putting a "global" If SBCL could be convinced to downgrade "unable to inline" from a style-warning to a note from a style-warning (either specifically for What should be a style-warning is using |
|
What's wrong with wrapping muffle conditions with asdf? The condition class for failure to inline is 'inlining-dependency-failure. You can use that to muffle of all inlining failure notes (and only those notes) if it's felt that all such failure notices are pointless. |
Quiet a few SBCL style warnings, mostly related to forward-referencing struct accessors that would otherwise be inlined.