Add support adding context instructions to basic block graphs. #289
Conversation
* Add fields to the `proto` specification to store context. * Add members to the Gematria `BasicBlock` data structure to store context and update methods on it and its Python binding accordingly. * Bonus: Remove dangling TODO.
| AddEdge(EdgeType::kStructuralDependency, previous_instruction_node, | ||
| instruction_node); |
There was a problem hiding this comment.
I think it would make sense to add a new edge type that connects the preceding context to the main block, and another one to connect the main block to the following context.
There was a problem hiding this comment.
I have an upcoming PR that adds EdgeType::kTakenBranch, which will always connect any preceding context with the main block and the main block with any following context when the LBR methodology is used to collect context (which also supports tracing multiple context blocks in either direction). Though, this does not necessarily have to be the case for any other data/context collection method.
There was a problem hiding this comment.
We could give the new edge type(s) a more generic name, e.g. kFromPrecedingContext and kToFollowingContext so that it doesn't suggest any specific type of context. But what I'd prefer to do is to give the model a signal where the boundary is.
SGTM for postponing this until a later PR.
| ElementsAre(true, false, false, true, false, true, false, true, | ||
| false, false, true, false)); | ||
| EXPECT_THAT(builder_->context_node_mask(), | ||
| ElementsAre(true, _, _, false, _, true, _, true, _, _, false, _)); |
There was a problem hiding this comment.
I'd prefer to check all the values (to check that the context and non-context parts are contiguous).
* Change `back_context` -> `preceding_context` and `front_context` -> `following_context`. * Add test for updated `BasicBlock::ToString`. * Keep previous proto field numbering.
* Update the graph builder and its Python bindings to add context instructions to basic block graphs and store context node mask to later be used by models. * Add tests for the new graph builder functionality.
* Change `back_context` -> `preceding_context` and `front_context` -> `following_context`. * Set `context_node_mask_` appropriately for non-instruction nodes as well, and update tests to reflect this.
virajbshah
force-pushed
the
block-context-graphs
branch
from
fcc10fd to
29fb29d
Compare
|
I have made some newer changes to the way After this change, this behavior is better defined with Please have a look. |
I've added one technical comment, but otherwise this makes sense. |
| @@ -291,49 +302,53 @@ bool BasicBlockGraphBuilder::AddInputOperand( | |||
| assert(instruction_node >= 0); | |||
| assert(instruction_node < num_nodes()); | |||
|
|
|||
| bool is_context = context_node_mask_[instruction_node]; | |||
There was a problem hiding this comment.
I'd rather pass this as an argument:
- it's probably faster (it will go through registers, with at least one fewer memory access),
- it reduces the risk that somewhere, sometimes we forget to set the the context mask for the instruction before adding its operands.
instructions to basic block graphs and store context node mask to later
be used by models.