From: Paul K. <pk...@gm...> - 2007-09-05 14:40:19
|
If I compile this reduced test case: (defun test-case (x) (labels ((inner () (values 1 0))) (if x (inner) (multiple-value-bind (a b) (inner) (values b a))))) (test-case t) => 1, 0 (test-case nil) => 0, 0 A disassembly of the function (at (speed 3) and (safety 0) to make it more readable for me) reveals this: ; 031412BC: 4881FA17001020 CMP RDX, 537919511 ; no-arg-parsing entry point ; 2C3: 752F JNE L0 ; 2C5: 488BC5 MOV RAX, RBP ; 2C8: 488BCC MOV RCX, RSP ; 2CB: 4883EC40 SUB RSP, 64 ; 2CF: 488941F8 MOV [RCX-8], RAX ; 2D3: 488BE9 MOV RBP, RCX ; 2D6: 488D0506000000 LEA RAX, [RIP+6] ; 2DD: 488945F0 MOV [RBP-16], RAX ; 2E1: EB11 JMP L0 ; 2E3: 488BCB MOV RCX, RBX ; 2E6: 488BD9 MOV RBX, RCX ; 2E9: 488D65F0 LEA RSP, [RBP-16] ; 2ED: 488B6DF8 MOV RBP, [RBP-8] ; 2F1: C20800 RET 8 ; 2F4: L0: B908000000 MOV ECX, 8 ; no-arg-parsing entry point ; 2F9: 31DB XOR EBX, EBX ; 2FB: 488D65F0 LEA RSP, [RBP-16] ; 2FF: 488B6DF8 MOV RBP, [RBP-8] ; 303: C20800 RET 8 Note the code at 2E3 and 2E6, which seems like a really bad way to exchange the contents of 2 registers :) Paul Khuong |
From: Nikodemus S. <nik...@ra...> - 2007-09-09 10:48:32
|
On 9/5/07, Paul Khuong <pk...@gm...> wrote: > (defun test-case (x) > (labels ((inner () > (values 1 0))) > (if x > (inner) > (multiple-value-bind (a b) > (inner) > (values b a))))) > > (test-case t) => 1, 0 > (test-case nil) => 0, 0 > ; 2E3: 488BCB MOV RCX, RBX > ; 2E6: 488BD9 MOV RBX, RCX > Note the code at 2E3 and 2E6, which seems like a really bad way to > exchange the contents of 2 registers :) Instrumenting MOV emission shows that these are MOVE-ARG VOPS. Quick look at places where we deal with VOP-INFO-MOVE-ARGS leads me to OK-COPY-REF, where the comment says ;;; Return true if ARG is a reference to a TN that we can copy ;;; propagate to. In addition to dealing with copy chains (as ;;; discussed below), we also discard references that are arguments ;;; to a local call, since IR2tran introduces temps in that context ;;; to preserve parallel assignment semantics. but the code does (not (and (eq (vop-info-move-args info) :local-call) (>= (or (position-in #'tn-ref-across arg (vop-args vop) :key #'tn-ref-tn) (error "Couldn't find REF?")) (length (template-arg-types info)))))))) Changing the AND to OR there seems to fix things, but since I'm not terribly familiar with our copy propagation, can someone verify that this is the right thing to do? Cheers, -- Nikodemus diff --git a/src/compiler/copyprop.lisp b/src/compiler/copyprop.lisp index 5a4e941..9e5d0f5 100644 --- a/src/compiler/copyprop.lisp +++ b/src/compiler/copyprop.lisp @@ -162,11 +162,11 @@ (unless (sset-member original in) (return nil))) (let ((info (vop-info vop))) - (not (and (eq (vop-info-move-args info) :local-call) - (>= (or (position-in #'tn-ref-across arg (vop-args vop) - :key #'tn-ref-tn) - (error "Couldn't find REF?")) - (length (template-arg-types info)))))))) + (not (or (eq (vop-info-move-args info) :local-call) + (>= (or (position-in #'tn-ref-across arg (vop-args vop) + :key #'tn-ref-tn) + (error "Couldn't find REF?")) + (length (template-arg-types info)))))))) ;;; Make use of the result of flow analysis to eliminate copies. We ;;; scan the VOPs in block, propagating copies and keeping our IN set |
From: Nikodemus S. <nik...@ra...> - 2007-09-10 12:38:42
|
On 9/9/07, Nikodemus Siivola <nik...@ra...> wrote: > Quick look at places where we deal with VOP-INFO-MOVE-ARGS leads > me to OK-COPY-REF, where the comment says > > ;;; Return true if ARG is a reference to a TN that we can copy > ;;; propagate to. In addition to dealing with copy chains (as > ;;; discussed below), we also discard references that are arguments > ;;; to a local call, since IR2tran introduces temps in that context > ;;; to preserve parallel assignment semantics. > > but the code does > > (not (and (eq (vop-info-move-args info) :local-call) > (>= (or (position-in #'tn-ref-across arg (vop-args vop) > :key #'tn-ref-tn) > (error "Couldn't find REF?")) > (length (template-arg-types info)))))))) > > Changing the AND to OR there seems to fix things, but since I'm not > terribly familiar with our copy propagation, can someone verify > that this is the right thing to do? I convinced myself that this is indeed TRT, and committed the change and Paul's test-case as 1.0.9.52. Cheers, -- Nikodemus |