Add a couple of variations of `traverse`. #1291

shirok · 2024-10-07T09:04:42Z

These are particularly useful in the optimizing path where traverser wants to have a broader view than a single subtree.

traverse-with-parent-thunk: In an optimizing pass, you sometimes neeed to know the parent of the node you're looking at. With this variation of traverse, the action functions receives a thunk after the node argument, which returns the parent node when called inside the function. An example is print-node-traversal-order.

traverse-with-hook: This is more general mechanism to intercept calling action procedures. traverse-with-parent-hook is implemented on top of this. Traverse hook is a function that is called whenever traverse recurse into subnodes. It allows to perform operations during traversal, anostic to how the node walker handle recursion for each node class.

stylewarning

Mostly small documentation-related comments.

stylewarning · 2024-10-07T09:43:18Z

src/codegen/traverse.lisp

        (apply *traverse* initial-node initial-args)))))

+(defun identity-hook (thunk node)


nit: I suggest renaming to identity-traverse-hook

src/codegen/traverse.lisp

stylewarning · 2024-10-07T09:46:09Z

src/codegen/traverse.lisp

+  "Default traversal hook function, which doesn't do any extra operation."
+  (declare (ignore node)) (funcall thunk))
+
+(defvar *traverse-hook* #'identity-hook)


Add documentation, but be explicit that traverse-with-hook should be used.

stylewarning · 2024-10-07T09:46:39Z

src/codegen/traverse.lisp

+  (let ((*traverse-hook* hook))
+    (apply #'traverse node action-list args)))
+
+;; Used in traverse-with-parent-ndoe


stylewarning · 2024-10-07T09:47:50Z

src/codegen/traverse.lisp

+             (let ((*traverse-parent-node-parent* *traverse-parent-node-current*)
+                   (*traverse-parent-node-current* node))
+               (funcall thunk))))
+    (let ((*traverse-hook* #'track-parent-node-hook))


traverse-with-hook can be used here, no?

Right. That was the original intention but somehow I forgot, I guess.

stylewarning

there is a small typo, but otherwise lgtm

stylewarning · 2024-10-07T17:37:24Z

src/codegen/traverse.lisp

+
+(defvar *traverse-hook* #'identity-traverse-hook
+  "A private dynamic variable used by `traverse-with-hook`.  This is rebound
+dyncamically during running `traverse-with-hook`.")


`traverse-with-parent-thunk`: In an optimizing pass, you sometimes neeed to know the parent of the node you're looking at. With this variation of `traverse`, the action functions receives a thunk after the node argument, which returns the parent node when called inside the function. An example is `print-node-traversal-order`. `traverse-with-hook`: This is more general mechanism to intercept calling action procedures. `traverse-with-parent-hook` is implemented on top of this. Traverse hook is a function that is called whenever `traverse` recurse into subnodes. It allows to perform operations during traversal, anostic to how the node walker handle recursion for each node class.

amorphedstar

This is a pretty cool idea and it's nice that you were able to do it quite concisely!
There were a few minor typos, but I am mostly curious to see if there is a better example where the extra hook is actually needed.

src/codegen/traverse.lisp

amorphedstar · 2024-10-08T03:43:29Z

src/codegen/traverse.lisp

+The `traverse` function is equivalent to this with passing
+`identity-traverse-hook` as the hook.
+
+One of the use cases is to record parent node of the node being traversed.


Suggested change

One of the use cases is to record parent node of the node being traversed.

One of the use cases is to record the parent node of the node being traversed.

amorphedstar · 2024-10-08T03:43:51Z

src/codegen/traverse.lisp

+(defun traverse-with-parent-thunk (node action-list &rest args)
+  "Like 'traverse', but actions are called with a thunk in the first argument,
+which returns the node's parent node.
+This captures one of the typical use case of `traverse-with-hook`."


Suggested change

This captures one of the typical use case of `traverse-with-hook`."

This captures one typical use case of `traverse-with-hook`."

amorphedstar · 2024-10-08T03:58:35Z

src/codegen/traverse.lisp

+The `traverse` function is equivalent to this with passing
+`identity-traverse-hook` as the hook.


I think that this is not quite true. Calling traverse does not revert *traverse-hook* to be identity-traverse-hook if it has already been redefined at the point where the traverse is called, while calling this function with identity-traverse-hook will make sure that the hook is reverted to identity-traverse-hook for the duration of the (sub-)traversal.

I guess this could be fixed by renaming the old traverse to something else, and making a new traverse which actually does pass identity-traverse-hook as a hook to this function. (Maybe the newer traverse and traverse-with-hook can be one function, just with the hook defaulting to identity-traverse-hook?)

Right. I didn't think the case that traverse is called during other traverse-family functions, but it is certainly possible.
Is there already a case that a function called from an action recursively calls traverse, thus can be affected with this commit?
Otherwise, I'd just fix this docstring.

Yes, the inliner definitely needs other sub-traversals of the code to be inlined, in order to rename type variables to fresh ones, as well as to perform AST variable substitution.

amorphedstar · 2024-10-08T04:27:34Z

src/codegen/traverse.lisp

+(defun print-node-parent (node)
+  "Print visiting node and its parent, using `traverse-with-parent-thunk`."
+  (declare (type node node)
+           (values node &optional))
+  (let ((counter 0))
+    (traverse-with-parent-thunk
+     node
+     (list
+      (action (:before node node parent-thunk)
+        (format t "PRE:  ~v@{|   ~}~A ~A~%" counter
+                (class-name (class-of node))
+                (class-name (class-of (funcall parent-thunk))))
+        (incf counter)
+        (values))
+      (action (:after node node parent-thunk)
+        (decf counter)
+        (format t "POST: ~v@{|   ~}~A ~A~%" counter
+                (class-name (class-of node))
+                (class-name (class-of (funcall parent-thunk))))
+        (values))))))


As far as I can tell, identical behavior can be achieved without traversing with the extra hook:
(not actually a code suggestion, just formatting it as one for easy comparison)

Suggested change

(defun print-node-parent (node)

"Print visiting node and its parent, using `traverse-with-parent-thunk`."

(declare (type node node)

(values node &optional))

(let ((counter 0))

(traverse-with-parent-thunk

node

(list

(action (:before node node parent-thunk)

(format t "PRE: ~v@{| ~}~A ~A~%" counter

(class-name (class-of node))

(class-name (class-of (funcall parent-thunk))))

(incf counter)

(values))

(action (:after node node parent-thunk)

(decf counter)

(format t "POST: ~v@{| ~}~A ~A~%" counter

(class-name (class-of node))

(class-name (class-of (funcall parent-thunk))))

(values))))))

(defun print-node-parent (node)

"Print visiting node and its parent, using `traverse-with-parent-thunk`."

(declare (type node node)

(values node &optional))

(let ((traversal-stack nil))

(traverse

node

(list

(action (:before node node)

(format t "PRE: ~v@{| ~}~A ~A~%" (length traversal-stack)

(class-name (class-of node))

(class-name (class-of (car traversal-stack))))

(push node traversal-stack)

(values))

(action (:after node node)

(pop traversal-stack)

(format t "POST: ~v@{| ~}~A ~A~%" (length traversal-stack)

(class-name (class-of node))

(class-name (class-of (car traversal-stack))))

(values))))))

Can you think of any other examples, where some functionality can only be achieved conveniently with traverse-with-hook, but not with the original traverse?

You're completely right. Merely obtaining parent can be achieved by this.
What I really need is a convenient API to access to the parent node, so we may avoid employing *traverse-hook* hack. Let me think on it.

shirok · 2024-10-08T20:40:41Z

@amorphedstar The original motivation is to get parent node during traversal. We didn't want a big surgery to traverse, though, for we might need other variations in future. So the idea of hook came. It is a bit hacky, but extensible.

I don't see other applications immediately, but what that might be needed for optimizations can be:

Make the path (from the root node to the current node) available
Gather information of subtrees of a particular type of node. E.g. specific type of node sets a table in a dynamic variable, and the descendant nodes add info to it.

shirok · 2024-10-08T20:57:31Z

I looked at #1291 (comment) and actually what I wanted could be achievable without *traverse-hook*. Let's see if I can make a convenient API around your idea.

shirok · 2024-10-09T12:40:55Z

@amorphedstar Need some input. I like your approach of keeping traversal-stack local, but what I want to achieve is traverse-like API that takes an action list, but each action gets the parent node info in the argument.

I'm trying to write traverse-with-path, which passes a thunk to return the "path" (traversal-stack in your code) to each actions, so that I can use it as follows:

(defun print-node-parent (node)
  "Print visiting node and its parent, using `traverse-with-path`."
  (declare (type node node)
           (values node &optional))
  (traverse-with-path
   node
   (list
    (action (:before node node path-thunk)
      (format t "PRE:  ~v@{|   ~}~A ~A~%" (length (funcall path-thunk))
              (class-name (class-of node))
              (class-name (class-of (car (funcall path-thunk)))))
      (values))
    (action (:after node node path-thunk)
      (format t "POST: ~v@{|   ~}~A ~A~%" (length (funcall path-thunk))
              (class-name (class-of node))
              (class-name (class-of (car (funcall path-thunk)))))
      (values)))))

But I need to use (:before node) and (:after node) action in traverse-with-path to push/pop the path. Here's an attempt to do so, which does not work if the caller also passes actions over generic nodes---either one is shadowed.

(defun traverse-with-path (node action-list &rest args)
  "Like 'traverse', but actions receives a thunk that reutnrs a reverse
list of the current node's ascendants following the current node.
For example, `car` of the ascendants is the node's immediate parent."
  (declare (type node node)
           (values node &optional))
  (let* ((traversal-path nil)
         (wrapped-action-list
           `(,(action (:before node node _path-thunk)
                (declare (ignore _path-thunk))
                (push node traversal-path)
                node)
              ,@action-list
              ,(action (:after node node _path-thunk)
                 (declare (ignore _path-thunk))
                 (pop traversal-path)
                 node))))
    (apply #'traverse node wrapped-action-list
           (lambda () (cdr traversal-path))
           args)))

Any ideas?

shirok · 2024-10-09T22:26:23Z

Found a workaround. Will submit another PR that supersedes this.

shirok · 2024-10-10T01:43:23Z

Superseded by #1300. Closing.

stylewarning requested a review from amorphedstar October 7, 2024 09:41

stylewarning requested changes Oct 7, 2024

View reviewed changes

shirok force-pushed the traverse-hook branch 2 times, most recently from fd99228 to 393b36f Compare October 7, 2024 11:21

stylewarning approved these changes Oct 7, 2024

View reviewed changes

shirok force-pushed the traverse-hook branch from 393b36f to 292acfe Compare October 7, 2024 20:38

amorphedstar reviewed Oct 8, 2024

View reviewed changes

shirok mentioned this pull request Oct 10, 2024

Add traverse-with-path #1300

Open

shirok closed this Oct 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a couple of variations of `traverse`. #1291

Add a couple of variations of `traverse`. #1291

shirok commented Oct 7, 2024

stylewarning left a comment

stylewarning Oct 7, 2024

stylewarning Oct 7, 2024

stylewarning Oct 7, 2024

stylewarning Oct 7, 2024

shirok Oct 7, 2024

stylewarning left a comment

stylewarning Oct 7, 2024

amorphedstar left a comment

amorphedstar Oct 8, 2024

amorphedstar Oct 8, 2024

amorphedstar Oct 8, 2024

shirok Oct 8, 2024

amorphedstar Oct 8, 2024

amorphedstar Oct 8, 2024

shirok Oct 8, 2024

shirok commented Oct 8, 2024

shirok commented Oct 8, 2024

shirok commented Oct 9, 2024

shirok commented Oct 9, 2024

shirok commented Oct 10, 2024

		(apply traverse initial-node initial-args)))))

		(defun identity-hook (thunk node)

	One of the use cases is to record parent node of the node being traversed.
	One of the use cases is to record the parent node of the node being traversed.

	This captures one of the typical use case of `traverse-with-hook`."
	This captures one typical use case of `traverse-with-hook`."

		The `traverse` function is equivalent to this with passing
		`identity-traverse-hook` as the hook.

Add a couple of variations of traverse. #1291

Add a couple of variations of traverse. #1291

Conversation

shirok commented Oct 7, 2024

stylewarning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stylewarning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amorphedstar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shirok commented Oct 8, 2024

shirok commented Oct 8, 2024

shirok commented Oct 9, 2024

shirok commented Oct 9, 2024

shirok commented Oct 10, 2024

Add a couple of variations of `traverse`. #1291

Add a couple of variations of `traverse`. #1291