Skip to content
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.

Sign up for GitHub

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

chore: SSA Refactor IR Review [DO NOT MERGE] #1896

Closed
wants to merge 1 commit into from
Closed

chore: SSA Refactor IR Review [DO NOT MERGE] #1896

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion crates/noirc_evaluator/src/ssa_refactor/ir/dfg.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,9 @@ pub(crate) struct DataFlowGraph {
intrinsics: HashMap<Intrinsic, ValueId>,

/// Contains each foreign function that has been imported into the current function.
/// This map is used to ensure that the ValueId for any given foreign functôn is always
/// This map is used to ensure that the ValueId for any given foreign function is always
/// represented by only 1 ValueId within this function.
/// TODO: can we merge this into intrinsics?
foreign_functions: HashMap<String, ValueId>,
Comment on lines -58 to 61
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must have missed this change when it was merged. I don't think we should ever key any of these maps by a String since it is unclear what it represents (a function name? Any arbitrary one, or one of a set of expected names? Are they expected to be extended later? etc).
I'd be in favor of creating a dedicated enum for foreign_functions (oracles? We use oracle elsewhere so we should be consistent in the naming), or merging them into the intrinsic enum.


/// Function signatures of external methods
Expand All @@ -68,6 +69,7 @@ pub(crate) struct DataFlowGraph {
/// Debugging information about which `ValueId`s have had their underlying `Value` substituted
/// for that of another. This information is purely used for printing the SSA, and has no
/// material effect on the SSA itself.
/// TODO: Is this true? How are we doing aliasing?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer true, looks like the comment has been stale for a while now. It should be updated.

replaced_value_ids: HashMap<ValueId, ValueId>,
}

Expand Down Expand Up @@ -103,6 +105,8 @@ impl DataFlowGraph {
/// block's id.
///
/// The pairs are order by id, which is not guaranteed to be meaningful.
/// TODO: do we need to order by reverse post order? Is this useful? Check brillig
/// TODO: ie do we need to get all successors/predecessors first?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is just for when a pass or test needs to iterate through each block and doesn't particularly care about the order (or whether the blocks are even still reachable). For passes that do care about the order, they can create a PostOrder directly like the Dead Instruction Elimination pass does.

pub(crate) fn basic_blocks_iter(
&self,
) -> impl ExactSizeIterator<Item = (BasicBlockId, &BasicBlock)> {
Expand All @@ -115,6 +119,7 @@ impl DataFlowGraph {
}

/// Inserts a new instruction into the DFG.
///
/// This does not add the instruction to the block.
/// Returns the id of the new instruction and its results.
///
Expand All @@ -132,13 +137,18 @@ impl DataFlowGraph {
}

/// Inserts a new instruction at the end of the given block and returns its results
///
/// TODO: This is doing more than just inserting an instruction and its results
Comment on lines +140 to +141
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like the missing piece is that this method attempts to simplify the instruction to another value and only inserts it if it could not do so. We should:

  1. Update the doc comment above to note this.
  2. Potentially rename the function? I'm unsure what a clearer name would be.

pub(crate) fn insert_instruction_and_results(
&mut self,
instruction: Instruction,
block: BasicBlockId,
ctrl_typevars: Option<Vec<Type>>,
) -> InsertInstructionResult {
use InsertInstructionResult::*;
// TODO: Is simplify still inserting the instruction?
// TODO: or are we assuming that simplify will always return a ValueId that
// TODO: has already been inserted?
Comment on lines +149 to +151
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplify may potentially simplify an instruction to an existing, known value (not necessarily a constant). E.g. add x, 0 is simplified to x. So it is expected simplify will always return a valid ValueId if it returns SimplifyResult::SimplifiedTo.

match instruction.simplify(self, block) {
SimplifyResult::SimplifiedTo(simplification) => SimplifiedTo(simplification),
SimplifyResult::Remove => InstructionRemoved,
Expand Down
4 changes: 2 additions & 2 deletions crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use super::{
/// Dominator tree node. We keep one of these per reachable block.
#[derive(Clone, Default)]
struct DominatorTreeNode {
/// The block's idx in the control flow graph's reverse post-order
/// The block's index in the control flow graph's reverse post-order
reverse_post_order_idx: u32,

/// The block that immediately dominated that of the node in question.
Expand Down Expand Up @@ -59,7 +59,7 @@ impl DominatorTree {
/// Returns the immediate dominator of `block_id`.
///
/// A block is said to *dominate* `block_id` if all control flow paths from the function
/// entry to `block_id` must go through the block.
/// entry to `block_id` must go through that block.
///
/// The *immediate dominator* is the dominator that is closest to `block_id`. All other
/// dominators also dominate the immediate dominator.
Expand Down
1 change: 1 addition & 0 deletions crates/noirc_evaluator/src/ssa_refactor/ir/function_inserter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use super::{
/// The FunctionInserter can be used to help modify existing Functions
/// and map old values to new values after re-inserting optimized versions
/// of old instructions.
/// TODO: optimized meaning calls to the simplify method et al?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"optimized" here just meaning a new instruction that some optimization pass wants to map to an existing instruction. For example, loop unrolling will re-insert each instruction in the body of a loop N times and it must keep track of this mapping each time in case the old instruction id is referenced later in the same loop iteration.

The new instruction that is mapped to is generally expected to either be an equivalent instruction or a more optimized form but I suppose it isn't strictly necessary as long as both have the same number of results.

pub(crate) struct FunctionInserter<'f> {
pub(crate) function: &'f mut Function,

Expand Down
12 changes: 9 additions & 3 deletions crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,7 @@ impl Instruction {
}
}
Instruction::Constrain(value) => {
// If the constraint value is known to be true, then we can remove the constraint instruction.
if let Some(constant) = dfg.get_numeric_constant(*value) {
if constant.is_one() {
return Remove;
Expand All @@ -277,10 +278,13 @@ impl Instruction {
let array = dfg.get_array_constant(*array);
let index = dfg.get_numeric_constant(*index);

// If we are getting a value from a constant array with a constant index
// Then we can substitute that value in place of the array_get instruction.
// TODO: what is a non-constant array?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A non-constant array is any value of Type::Array for which we don't know the exact values of each element. For example, in v1 = array_set v0, index Field 1, value Field 7, v1 (and v0) are non-constant since we do not know their element values(*). Note that non-constant arrays include more than just "arrays that are parameters to main" because it also includes arrays that may eventually become known constants by later compiler passes.

(*) In the SSA printer if an array is constant the underlying array itself [0, 1, 2, etc] is printed, and if it isn't constant just the ID v1 is printed. So if you see an ID instead of an array value you know it is not constant.

if let (Some((array, _)), Some(index)) = (array, index) {
let index =
index.try_to_u64().expect("Expected array index to fit in u64") as usize;

// TODO: Where do we check for Out of Bounds(OOB) error?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only check for OOB during acir-gen. Any checks that are known to be out of bounds during SSA are caught right here and just left in the code for acir-gen to find. It doesn't particularly matter where we stop to report these errors as long as that spot eventually has location information for them.

if index < array.len() {
return SimplifiedTo(array[index]);
}
Expand Down Expand Up @@ -379,6 +383,7 @@ fn simplify_call(func: ValueId, arguments: &[ValueId], dfg: &mut DataFlowGraph)
arguments.iter().map(|value_id| dfg.get_numeric_constant(*value_id)).collect();
let constant_args = match constant_args {
Some(constant_args) => constant_args,
// Not a big fan of adding SimplifyResult to the namespace because of the None variant which looks like Option::None
TomAFrench marked this conversation as resolved.
Show resolved Hide resolved
Option::None => return None,
};
match intrinsic {
Expand All @@ -393,6 +398,7 @@ fn simplify_call(func: ValueId, arguments: &[ValueId], dfg: &mut DataFlowGraph)
let limb_count = constant_args[2].to_u128() as u32;
SimplifiedTo(constant_to_radix(endian, field, radix, limb_count, dfg))
}
// TODO: Can simplify here too!
Intrinsic::BlackBox(_) | Intrinsic::Println | Intrinsic::Sort => None,
TomAFrench marked this conversation as resolved.
Show resolved Hide resolved
}
}
Expand Down Expand Up @@ -421,7 +427,7 @@ fn constant_to_radix(
}

// For legacy reasons (see #617) the to_radix interface supports 256 bits even though
// FieldElement::max_num_bits() is only 254 bits. Any limbs beyond the specified count
// FieldElement::max_num_bits() is only 254 bits for bn254. Any limbs beyond the specified count
// become zero padding.
let max_decomposable_bits: u32 = 256;
let limb_count_with_padding = max_decomposable_bits / bit_size;
Expand All @@ -442,7 +448,7 @@ pub(crate) enum InstructionResultType {
Known(Type),

/// The result type of this function is unknown and separate from its operand types.
/// This occurs for function calls and load operations.
/// TODO:WHY? This occurs for function calls and load operations.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given Instruction::Load(v0) what is the result type? The type of v0 is just Type::Reference so we cannot know since the element type is not specified.

Given Instruction::Call { f, args } what is the result type? The type of f here again is just Type::Function without specifying argument or return types so we cannot know.

If we flesh out Type::Reference or Type::Function to include this information we can get rid of this variant. The original reasoning was to keep types simple and trivially copyable but we've already abandoned that with Type::Array { .. } no longer being copyable.

Unknown,

/// This instruction does not return any results.
Expand Down
2 changes: 1 addition & 1 deletion crates/noirc_evaluator/src/ssa_refactor/ir/value.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ pub(crate) enum Value {
Intrinsic(Intrinsic),

/// This Value refers to an external function in the IR.
/// ForeignFunction's always have the type Type::Function and have simlar semantics to Function,
/// ForeignFunction's always have the type Type::Function and have similar semantics to Function,
/// other than generating different backend operations and being only accessible through Brillig.
ForeignFunction(String),
}
Expand Down
Loading