47 bytes, text/x-phabricator-request
|Details | Review|
No description provided.
Want to run this comment by you first, as you're the most recent person to try to puzzle through some of this. If you think it's helpful, then I'll get it reviewed by Ted or Jan.
Attachment #9027281 - Flags: feedback?(iireland)
Assignee: nobody → mgaudet
Status: NEW → ASSIGNED
Comment on attachment 9027281 [details] [diff] [review] Comment CacheIR value tracking f?iain Review of attachment 9027281 [details] [diff] [review]: ----------------------------------------------------------------- This looks generally quite helpful. You pointed out a couple of times in our discussion that the RAII nature of AutoScratchReg et al lets you do tricks with internal scopes. Should that be pointed out somewhere in this comment? ::: js/src/jit/CacheIRCompiler.h @@ +132,5 @@ > +// OperandIds are created in the CacheIR front-end to keep track of values that > +// are needed to create the given IC stub. In the CacheRegisterAllocator, these > +// OperandIds are given OperandLocations, which map the logical value (the > +// OperandId) to the physical location of the value at a given point in time > +// during CacheRegister allocation. "values that are needed to create the given IC stub" Is this distinct from some other type of value (eg "values needed to create" vs "values needed to execute"), or is it just saying that we need values to create stubs? If the latter, it might be clearer to just say "OperandIds are created ... to keep track of logical values". And then something along the lines of "In the CacheRegisterAllocator, these OperandIds are given OperandLocations, which represent the physical location of the value at a given point in time." (This is just virtual registers and physical registers, right? Maybe it would be good to point that out explicitly?) @@ +144,5 @@ > +// There are a number of RAII classes that interact with the register allocator > +// > +// - AutoOutputReg > +// - AutoScratchReg > +// - AutoScratchRegMaybeOutput Are these documented in more detail anywhere else? If not, this might be a good place to do so. @@ +148,5 @@ > +// - AutoScratchRegMaybeOutput > +// > +// These RAII classes take ownership of a register for the duration of their > +// lifetime so they can be used for computation or output. The register > +// allocator can spill named operands in order to try to ensure that a register What is a "named" operand in this context? @@ +154,5 @@ > +// > +// If a register is not available, currently a MOZ_RELEASE_ASSERT ensures the > +// browser will crash, as we cannot statically guarantee enough registers (and > +// in fact we are on occasion stymied by the small number of registers available > +// on x86-32) This could be a little bit clearer on the relationship between spilling and the MOZ_RELEASE_ASSERT. "If no register is available to be spilled"? @@ +159,5 @@ > +// > +// FailurePaths checkpoint the state of the register allocator so that the > +// input state can be recomputed from the current state before jumping to > +// the next stub in the IC chain. An important, but not currently enforced > +// through assertion or type, invariant is that the FailurePath must be Quibble: Splitting up "important" and "invariant" here is kind of awkward. @@ +169,5 @@ > +// The RAII register management classes are RAII because all register state > +// outside the OperandLocations is reset before the compilation of each > +// individual CacheIR op. This means that you cannot rely on a value surviving > +// between ops, even if you use the ability of AutoScratchRegister to name a > +// specific register. "You cannot rely on a value surviving between ops" should be something like "values that should be preserved between ops must be given an OperandId", right? Or "you cannot rely on a register being preserved between ops"? (Also, whitespace.)
Attachment #9027281 - Flags: feedback?(iireland) → feedback+
Comment on attachment 9027281 [details] [diff] [review] Comment CacheIR value tracking f?iain Hopefully the Phabricator patch addresses all the feedback (and thanks very much for said feedback!)
Attachment #9027281 - Attachment is obsolete: true
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/9a9e55ac2f08 Comment CacheIR value tracking r=jandem
You need to log in before you can comment on or make changes to this bug.