-
Notifications
You must be signed in to change notification settings - Fork 73
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
Cleanups having to do with fidelity of sets of instructions #916
base: master
Are you sure you want to change the base?
Cleanups having to do with fidelity of sets of instructions #916
Conversation
72b4eba
to
75ea5ab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a once-over to check for obvious things. Looks pretty good.
In my initial review though I was a bit confused about using the minimum. It seemed the old code didn't actually calculate minima, but it mentioned it in a doc string?
I'd personally like some clarity on that.
Not approving or requesting changes until I understand it better.
(defun calculate-instructions-log-fidelity (instructions chip-specification) | ||
"Calculates the fidelity of a sequence of native INSTRUCTIONS on a chip with architecture governed by CHIP-SPECIFICATION (and with assumed perfect parallelization across resources)." | ||
(reduce #'+ instructions | ||
:key (lambda (instr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be silly but maybe we should rewrite the reduce as a loop to avoid the closure, or you should FLET it (it would have a nice name) and declare it dynamic-extent
SBCL is maybe smart enough to do this already; I don't know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could use loop - I was just too lazy to determine the container type of instructions
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opted for flet
instead: citing above laziness.
src/addresser/logical-schedule.lisp
Outdated
(let ((running-fidelity 0d0)) | ||
(map-lschedule-in-topological-order | ||
lschedule | ||
(lambda (instr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider flet + DX
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you mean by + DX
? Are you asking that I not use incf
? I can't tell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored to use flet
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DX is SBCLian slang for "dynamic extent (declaration)"
src/addresser/logical-schedule.lisp
Outdated
(let ((min-fidelity 1.0d0)) | ||
(map-lschedule-in-topological-order | ||
lschedule | ||
(lambda (instr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider flet + DX
src/chip/chip-specification.lisp
Outdated
(let (fidelity) | ||
(a:when-let* ((obj (lookup-hardware-object chip-spec instr)) | ||
(specs-hash (hardware-object-gate-information obj)) | ||
(binding (and (< 0 (hash-table-count specs-hash)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plusp
src/compressor/compressor.lisp
Outdated
|
||
(defun calculate-instructions-fidelity (instructions chip-specification) | ||
"Calculates the fidelity of a sequence of native INSTRUCTIONS on a chip with architecture governed by CHIP-SPECIFICATION (and with assumed perfect parallelization across resources)." | ||
(exp (- (calculate-instructions-log-fidelity instructions chip-specification)))) | ||
(reduce #'min instructions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this correct? why the minimum across instructions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes so, the old function here depended upon a mathematical trick that approximated the minimum fidelity by passing the negative sum of log squared of individual fidelities to an exponential function. What you got was not the minimum fidelity but was "close".
This change cuts out the middle man, gives you the minimum, and doesn't have to calculate the sum of log squared terms.
The function as it had appeared in compressor was more-or-less copied from its similarly named function in logical-schedule. The trick was explained there in the (old) docstring to lschedule-calculate-fidelity
. I.e.
min(x,y) ~ exp(-(sqrt (ln(x)^2 + ln(y)^2)))
.
I assume this trick was used in order to recycle the lschedule-calculate-log-fidelity
function. But that recycling isn't really worth it, in my opinion: it is much faster to just get the actual minimum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and I assume it wants the minimum because the doc string of the function that this function is directly imitating literally states that's what it is trying to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh. I think I'm originally responsible for this goofy expression, and I don't think I was looking for an approximate minimum — if x = y, for instance, you won't get min(x, y) = x but x^{-sqrt 2}. I also don't think I'm responsible for that old docstring; there was a refactor in 2019–2020 where someone else did their best to read the tea leaves. That said, I don't think there's any true justification to what I wrote, as fidelities do not easily compose, neither horizontally nor vertically. If you want something with at least some thin mathematical justification, you could consider using sum
everywhere, since infinitesimal infidelities do compose OK and we're all paid to be very optimistic about long-term hardware performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this is a way to use the composition-as-a-sum-of-log-squares and normalize it to [0,1] so that it lives in the same set as the fidelity scores?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is nice that it keeps to [0, 1]; I'm not sure if that's accidental or intentional. On the other hand, the minimum fidelity of an nQ gate is 1/(1 + 2^n), so maybe not that nice. And sums of infinitesimals won't escape [0, 1] either. I dunno.
Some of the changes I have made here have to do with removing needless logical schedule construction. The assessment of needlessness was based on looking at the code as it actually is (and determining that extra work was being done), but not necessarily what it was intended to be. Specifically, all of these fidelity calculations first construct a logical schedule, but then use that schedule to iterate over every instruction, adding up the (log squared) fidelities of each, and then either using that sum or using the But! maybe they're meant to be. Maybe it was decided that the logical schedule is the place to calculate fidelities b/c there may at some point be a better model of whole-program fidelity calculation that takes into account the logical ordering of operations. I now have some doubts about the eventual emergence of such a model based on what @ecpeterson mentioned above: that fidelities don't generally compose well. So, I don't know if this PR should remove the instantiation of logical schedules from the pipeline of instructions -> fidelity score because doing so may or may not be a violation of intended design. @stylewarning |
Something else to bear in mind is that the addresser has a cross-competing concern about instruction ordering: it wants to make good decisions so that the total resulting schedule has good fidelity, but it also needs to make a decision about what SWAPs to immediately insert, and so it can't get too wistful about instructions down the line. We dealt with this by adding a decay factor to logically-temporally distant instructions, which is fine for this scheduling application but less fine for calculating the total fidelity of a schedule. I only bring this up to say (1) that there is a place where we jam in ordering info into these values + (2) something like that jamming is unavoidable in greedy-ish scheduling, so it's always going to distort the compiler's internal perspective on these values, and so maybe it isn't so important to come up with a really nice (/ computationally expensive) formula. (Users' external perspectives on these values are another matter! They probably do want meaningful values.) |
Sorry I don't know if I made my concern clear. My question has to do with a very specific case: Several of these fidelity calculations start with a sequence of instructions, build a logical schedule out of them, calculate fidelity, then throw the recently built logical schedule away. In these situations, the logical schedule is only being built so that it can be passed to a fidelity calculation routine. (This is the situation in some of the compressor code as well as a sub-step in the fidelity-addresser code. ) Maybe this is fine if what you want to do is to take into account the ordering of operations in your calculation of fidelity. Right now that is not happening - the actual computation doesn't take order into account. But perhaps one might in the future expect order to be significant - if that is the case, it makes sense to create disposable logical schedules. I just don't know if there is a meaningful way to think about fidelity on a logical schedule that doesn't also apply to simple sequences of instructions. |
I myself think it is fine not to bother calculating these small logical schedules and to use a fairly dumb fidelity estimate which need not rely on logical ordering. One last comment: Certain Places care foremost about coherence limitations and so only really care about the “longest chain” of instructions. I guess this is comparable to what we’re doing with the temporal strategy? It would be nice to have a similarly clear objective for the “fidelity strategy” if it’s not, in fact, computing fidelities. |
Move defs near to use sites; refactor instrs fidelity Fidelity calculations used by the compressor and by the fidelity addresser were needlessly constructing logical schedules. Peeking at the logical schedule fidelity calculations, they too have been cleaned up and refatored for clarity and performance. Generally opting for treating fidelity of a circuit as the minimum fidelity of its constituent gates. Prior to this commit, when a composite fidelity needed to fall in the range (0, 1] the calculation (exp (- (sqrt (reduce #'+ fidelities :key (lambda (f) (* (log f) (log f))))))) was being used. For fidelities f in (0, 1] this produced a non-negative value f0 that was at most the minimal f.
a1a3cdd
to
3f281cf
Compare
I opted to replace most of the
calculations with a simpler
expression. Both will yield values in the interval The first has the added benefit of getting worse as more "bad" fidelities enter and so is perhaps more reflective of compounding bad-upon-bad. The downside is that more computation is required for empirically uncertain benefit. Today I have pushed the most recent version of these changes as a kind of indication of life. I am considering what next to do. Some thoughts that have occurred to me:
|
Move defs near to use sites; refactor instrs fidelity
Fidelity calculations used by the compressor and by the fidelity addresser were needlessly constructing logical schedules.
Peeking at the logical schedule fidelity calculations, they too have been cleaned up and refatored for clarity and performance.