Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unusual control flow rarely screws up contify #13

Open
Bike opened this issue Sep 8, 2021 · 1 comment
Open

Unusual control flow rarely screws up contify #13

Bike opened this issue Sep 8, 2021 · 1 comment
Labels
bug Something isn't working

Comments

@Bike
Copy link
Member

Bike commented Sep 8, 2021

;; If the return-point has a predecessor, it does not start a
;; block and will be the unique outside call to this
;; function, which means we should normalize the return point
;; to be a dummy block.
(unique-call
(and (not (eq return-point :unknown))
(cleavir-bir:predecessor return-point)))
(return-point
(if unique-call
(progn
(check-type unique-call cleavir-bir:local-call)
(let ((dummy-block (nth-value 1 (cleavir-bir:split-block-after unique-call)))
(ucall-out (cleavir-bir:output unique-call)))
(unless (cleavir-bir:unused-p ucall-out)
(let ((phi (make-instance 'cleavir-bir:phi :iblock dummy-block)))
(setf (cleavir-bir:inputs dummy-block) (list phi))
;; Replace the call-as-datum with the return-values.
(cleavir-bir:replace-uses phi ucall-out)))
dummy-block))
(if (eq return-point :unknown)
:unknown
(cleavir-bir:iblock return-point))))

Here, contify only recognizes two situations. Either there are multiple calls, and the results of the call (if used) are fed into a shared phi; or, there is only one call, and the return point is the successor of the call. If the return point is the beginning of an iblock, it assumes that it is in the former situation. In this situation, in order to communicate the return values of the inlined function, that iblock's phi, assumed to be the shared results, is used.

However, it is possible to contrive things so that the return point is the beginning of an iblock even if there is only one call, and in a way such that the results of the call are actually needed. Here is the smallest example I could come up with, reduced from clasp-developers/clasp#1181:

(lambda (reqs opts)                                                                                                            
  (values                                                                                                         
   (BLOCK NIL
     (LET* ((LOOP-LIST-HEAD27101 (LIST NIL))
            (LOOP-LIST-TAIL27102 LOOP-LIST-HEAD27101))
       (TAGBODY
        NEXT-LOOP
          (return-from nil (cdr loop-list-head27101))
          (GO NEXT-LOOP))))

   ;; CHECK-TYPE optional parameters                                                                                                                               
   (TAGBODY
      (GO G27058)
    G27058
      (when opts (GO G27058)))))

For this code, the results of the call to cdr (which is inlined) are used in the final return value. However, the immediate successor of the call, after catch reduction, is a jump to G27058, and the return-point is then the first instruction in that iblock. This "tricks" the contifier into thinking it does not need to create a new iblock/phi. Then when it tries to delete the call, it finds that the call is still used, and the assertion in the delete function ((EVERY CLEAVIR-BIR:UNUSED-P (CLEAVIR-BIR:OUTPUTS CLEAVIR-BIR:INSTRUCTION))) fails.

@Bike Bike added the bug Something isn't working label Sep 8, 2021
@Bike
Copy link
Member Author

Bike commented Sep 9, 2021

The above characterization misses the fact that there are also tail recursive calls. A better way to put it would be that this is sort of a consequence of not having strict CPS. We have some calls that are in a similar form - they're immediately followed by a phi using their values, as from (if x (foo y) (foo z)). But we have some calls that are instead in the middle of blocks and have their values used directly. These need different approaches since we have to turn the latter case into the former case, but the way we distinguish the cases is wrong since a call in the second form can still be immediately succeeded by a jump that doesn't use its value.

Bike added a commit that referenced this issue Sep 10, 2021
This more properly distinguishes the single call from the multiple
call case and I think is correct, but I haven't proven that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant