[SMDOC] Document CacheIR value tracking and register allocation

RESOLVED FIXED in Firefox 65

Status

()

enhancement
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: mgaudet, Assigned: mgaudet)

Tracking

(Blocks 1 bug)

Trunk
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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

Updated

6 months ago
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.)

Updated

6 months ago
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

Comment 5

6 months ago
Pushed by mgaudet@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9a9e55ac2f08
Comment CacheIR value tracking r=jandem

Comment 6

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9a9e55ac2f08
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.