From efc9a618d564f00c4fd874f055c43961ac2f4fd9 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 19 Nov 2024 00:10:15 +0000 Subject: [PATCH] Add lifetime to Resolved so it is non-trivial to persist it --- .../src/brillig/brillig_gen.rs | 4 +-- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 6 ++-- .../src/ssa/ir/function_inserter.rs | 8 ++--- compiler/noirc_evaluator/src/ssa/ir/value.rs | 32 +++++++++++++------ .../noirc_evaluator/src/ssa/opt/array_set.rs | 4 +-- .../src/ssa/opt/mem2reg/block.rs | 2 +- 6 files changed, 34 insertions(+), 22 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen.rs index 30920fd300d..8e52d9b31db 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen.rs @@ -26,8 +26,8 @@ impl IsResolved for FinalResolved {} type FinalValueId = ValueId; -impl From for FinalValueId { - fn from(value: ResolvedValueId) -> Self { +impl From> for FinalValueId { + fn from(value: ResolvedValueId<'_>) -> Self { ValueId::new(value.raw()) } } diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 09862c10920..db319514d96 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -250,11 +250,11 @@ impl DataFlowGraph { /// is returned. pub(crate) fn resolve(&self, original_value_id: ValueId) -> ResolvedValueId { if R::is_resolved() { - return ResolvedValueId::new(original_value_id.raw()); + return ValueId::new(original_value_id.raw()); } match self.replaced_value_ids.get(original_value_id.as_ref()) { Some(id) => self.resolve(*id), - None => ResolvedValueId::new(original_value_id.raw()), + None => ValueId::new(original_value_id.raw()), } } @@ -350,7 +350,7 @@ impl DataFlowGraph { /// Resolve and get a value by ID fn resolve_value(&self, original_value_id: ValueId) -> &Value { - let id = self.resolve(original_value_id.unresolved()).raw(); + let id = self.resolve(original_value_id).raw(); &self.values[id] } diff --git a/compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs b/compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs index ae9681b5c06..431d06a97e4 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs @@ -43,11 +43,11 @@ impl<'f> FunctionInserter<'f> { /// Resolves a ValueId to its new, updated value. /// If there is no updated value for this id, this returns the same /// ValueId that was passed in. - pub(crate) fn resolve(&mut self, value: ValueId) -> ResolvedValueId { + pub(crate) fn resolve(&mut self, value: ValueId) -> ResolvedValueId<'f> { let value = self.function.dfg.resolve(value); match self.values.get(&value.raw()) { Some(value) => self.resolve(*value), - None => value, + None => ValueId::new(value.raw()), } } @@ -119,7 +119,7 @@ impl<'f> FunctionInserter<'f> { call_stack: CallStack, ) -> InsertInstructionResult { let results = self.function.dfg.instruction_results(id); - let results = vecmap(results, |id| self.function.dfg.resolve(*id)); + let results = vecmap(results, |id| self.function.dfg.resolve(*id).detach()); let ctrl_typevars = instruction .requires_ctrl_typevars() @@ -209,7 +209,7 @@ impl<'f> FunctionInserter<'f> { /// ValueId (from the source_function) and its new ValueId in the destination function. pub(crate) fn insert_new_instruction_results( values: &mut HashMap, - old_results: &[ResolvedValueId], + old_results: &[ResolvedValueId<'f>], new_results: &InsertInstructionResult, ) { assert_eq!(old_results.len(), new_results.len()); diff --git a/compiler/noirc_evaluator/src/ssa/ir/value.rs b/compiler/noirc_evaluator/src/ssa/ir/value.rs index 3654d7d122d..d81daaab05f 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/value.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/value.rs @@ -15,13 +15,24 @@ use super::{ #[derive(Debug, Clone, Serialize, Deserialize)] pub(crate) enum Unresolved {} -/// Resolved marker; doesn't implement `Hash` so it can't be stored in maps. +/// Marker for resolved status. +/// +/// Doesn't implement `Hash` so it can't be stored in maps. +/// It has a lifetime so it's not easy to store it in data structures, +/// where it could become stale. Instead we can implement module specific +/// variants when we can prove that persisting them is safe because the +/// IDs are not going to be changed between use. +/// #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize /* PartialOrd, Ord, Hash */)] -pub(crate) enum Resolved {} +pub(crate) struct Resolved<'a> { + _marker: PhantomData<&'a ()>, +} + +pub(crate) type ResolvedValueId<'a> = ValueId>; pub(crate) trait IsResolved {} -impl IsResolved for Resolved {} +impl<'a> IsResolved for Resolved<'a> {} pub(crate) trait Resolution { fn is_resolved() -> bool; @@ -39,9 +50,6 @@ impl Resolution for R { } } -/// A resolved value ID is something we can directly compare. -pub(crate) type ResolvedValueId = ValueId; - /// A raw value ID that can be used as a key in maps. pub(crate) type RawValueId = Id; @@ -79,8 +87,15 @@ impl ValueId { self.id == other.id } /// Promote an unresolved ID into a resolved one. - pub(crate) fn resolved(self) -> ValueId { - ValueId::new(Id::new(self.id.to_usize())) + pub(crate) fn resolved(self) -> ValueId> { + ValueId::new(self.id) + } +} + +impl<'a> ValueId> { + /// Change the lifetime of a resolution. + pub(crate) fn detach<'b>(self) -> ValueId> { + ValueId::new(self.id) } } @@ -119,7 +134,6 @@ impl From> for ValueId { value.unresolved() } } - impl From> for ValueId { fn from(value: Id) -> Self { ValueId::new(value) diff --git a/compiler/noirc_evaluator/src/ssa/opt/array_set.rs b/compiler/noirc_evaluator/src/ssa/opt/array_set.rs index 0a023b31e58..117f6147ba8 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/array_set.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/array_set.rs @@ -59,9 +59,7 @@ impl Function { #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] pub(super) enum ContextResolved {} -//impl IsResolved for ContextResolved {} - -impl From for ValueId { +impl From> for ValueId { fn from(value: ResolvedValueId) -> Self { ValueId::new(value.raw()) } diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs index 191a803bb9d..724cac91527 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs @@ -14,7 +14,7 @@ pub(super) enum ContextResolved {} impl IsResolved for ContextResolved {} -impl From for ValueId { +impl From> for ValueId { fn from(value: ResolvedValueId) -> Self { ValueId::new(value.raw()) }