freenode/#sicl - IRC Chatlog
Search
7:18:22
no-defun-allowed
Yeah, checking out before my changes makes it work again, but I don't recall modifying the boot process.
7:20:16
beach
You may have removed an initializing instruction when you did your HIR transformations.
7:21:10
no-defun-allowed
I didn't change the boot procedure, so it wouldn't use my transformations yet.
7:23:57
no-defun-allowed
No, I did, actually. I added a method for INSTRUCTION-MAY-BE-REMOVED-P that allows a SAVE-VALUES-INSTRUCTION instruction to be removed if the values aren't used, but that's evidently incorrect.
7:27:05
no-defun-allowed
"Right now, the output of this instruction is considered not used, so the instruction gets removed." And that would explain why.
7:28:10
no-defun-allowed
That was from the description comment above the definition of SAVE-VALUES-INSTRUCTION.
7:39:38
no-defun-allowed
Aha, it's not counted because an instruction using a location as the dynamic environment is not tracked as a use.
7:41:32
beach
Isn't it much simpler than that? The instruction has no explicit output, and that's how it is determined that an instruction can be removed. But the implicit output is important here, so the instruction cannot be removed.
7:43:34
no-defun-allowed
SAVE-VALUES has an output (the new dynamic environment), but RESTORE-VALUES doesn't have take an input (it takes that dynamic environment as an "input").
7:52:45
beach
I take your word for it. This stuff is no longer fresh in my memory. Unless, of course, you need my help with something.
7:53:35
no-defun-allowed
Well, when I go to inspect the HIR in the visualizer, dynamic environments are uses, but in partial inlining, it appears they aren't.
7:55:16
no-defun-allowed
I put in a call to REINITIALIZE-DATA before the stuff in DO-INLINING, and now uses are tracked correctly then.
8:01:09
no-defun-allowed
What I think is the culprit is that REINITIALIZE-DATA treats an instruction using a dynamic environment as a use, but the auxiliary methods on writers for the slots of instructions and initialize-instance do not.
8:14:12
no-defun-allowed
Yeah, without trying to eliminate STORE-INSTRUCTIONs that aren't used, bootstrapping works.
8:16:11
no-defun-allowed
But by my interpretation of "use" which includes the dynamic environment location, uses aren't being tracked consistently.
8:16:40
beach
That's possible. But I don't see the relation between that problem and eliminating the SAVE-VALUES-INSTRUCTION.
8:17:06
beach
And it sounds strange to me, because I depend on the dynamic environment being tracked correctly.
8:18:01
no-defun-allowed
REINITIALIZE-DATA treats the dynamic environment as a use, but initializing an instruction does not, nor do the non-present auxiliary methods for DYNAMIC-ENVIRONMENT-LOCATION (compare to those for INPUTS and OUTPUTS).
8:22:23
no-defun-allowed
Looking at the :after method for initialize-instance around line 118 of Cleavir2/Intermediate-representation/instruction.lisp, we see that the input and output locations have the instruction added as using as defining, respectively.
8:23:01
no-defun-allowed
Note that the dynamic environment location has no such using-instruction pushed.
8:23:56
no-defun-allowed
Now, compare this to line 137 (in reinitialize-data) of Cleavir2/Intermediate-representation/graph-modifications.lisp in which the dynamic environment location is used by the instruction.
8:24:00
beach
I am not even sure that tracking that information in instruction initialization is required anywhere.
8:24:45
beach
I am pretty sure that after any significant transformation of the HIR graph, there is a call to reinitialize-data.
8:25:26
beach
Though, I think the work by Bike and karlosz aims to change that so that there will be less need to do global work on the graph.
8:25:40
no-defun-allowed
That did not seem to be the case when partial inlining removes useless instructions.
8:27:28
karlosz
no-defun-allowed: yeah a lot of that stuff was bolted on since you could easily get cubic /in terms of number of hir instructions/ time operations on the graph
8:27:32
no-defun-allowed
I don't want to say it, but I think that relates to the incorrect removal of SAVE-VALUES-INSTRUCTIONs, as the new environment was not considered to be used at that point.
8:30:02
no-defun-allowed
I have added to initialization and new auxiliary methods on DYNAMIC-ENVIRONMENT-LOCATION (like those for INPUTS and OUTPUTS), and now it appears to behave.
8:32:05
no-defun-allowed
Now that uses are tracked correctly, I can remove the SIDE-EFFECT-MIXIN and use the normal method for deciding if an instruction is useless.
8:48:07
no-defun-allowed
I'm going to make a pull request, but I've somehow messed up git again, and rebasing against some commits I hadn't pulled before (possibly because of branch mismanagement) has caused all of those to be attributed with "robert-strandh authored and no-defun-allowed committed" which is odd.
8:52:23
scymtym
rebasing creates new commits when the "parents" or timing information of commits change. they can have the same author and represent the same change as an existing commit, but the "committer" of the new commits is the person doing the rebasing
8:53:01
no-defun-allowed
Okay, I rebased again and now it looks reasonable. scymtym's explanation matches up with what I saw.
9:08:08
beach
Thanks. I'll merge it and then check that the new bootstrapping procedure is still working.
9:11:53
no-defun-allowed
Well, we were before, depending on which function you asked. But now we're definitely tracking it.
9:13:19
heisig
I thought my HIR-evaluator was treating them incorrectly, but I had just messed up closure creation entirely and those were the first locations to expose that bug.
9:13:54
heisig
But I haven't heard complaints about the evaluator since, so I hope things are fine now.