Skip to content

Commit c9546ae

Browse files
jacob-shopsjacob-shops
committed
Remove references to dead store elimination
1 parent fa72cb8 commit c9546ae

2 files changed

Lines changed: 86 additions & 89 deletions

File tree

‎zjit/src/hir.rs‎

Lines changed: 86 additions & 88 deletions
Original file line numberDiff line numberDiff line change
@@ -5283,92 +5283,92 @@ impl Function {
52835283
// other HIR instructions before a second store to the same offset occurs.
52845284
// Removing one of these two stores does not alter program behavior.
52855285
// TODO: Improve comments and clean up algorithm sketch
5286-
fn eliminate_dead_stores(&mut self) {
5287-
#[derive(PartialEq, Eq, Hash)]
5288-
struct StoreHeap {
5289-
// TODO: Is object a good name here? If not, what should it be instead?
5290-
object: InsnId,
5291-
offset: i32,
5292-
}
5293-
5294-
// This helper function is used while we don't have type based alias
5295-
// analysis. Matching offsets could result in aliased objects that need
5296-
// to be clear.
5297-
fn prune_stores(map: &mut HashMap<StoreHeap, InsnId>, offset: i32) {
5298-
map.retain(|StoreHeap { object: _, offset: off }, _| *off != offset);
5299-
}
5300-
5301-
for block in self.rpo() {
5302-
// TODO: Probably convert this to a vec of tuples? Time complexity is worse but constant factors are better
5303-
let mut active_stores: HashMap<StoreHeap, InsnId> = HashMap::new();
5304-
let insns = std::mem::take(&mut self.blocks[block.0].insns);
5305-
let mut new_insns = vec![];
5306-
for insn_id in insns.iter().rev().copied() {
5307-
match self.find(insn_id) {
5308-
Insn::StoreField { recv, offset, .. } => {
5309-
let key = StoreHeap { object: self.chase_insn(recv), offset };
5310-
// If an older store is being tracked, then there have
5311-
// been no usees between stores and the first store is
5312-
// dead and should not be emitted.
5313-
if active_stores.contains_key(&key) {
5314-
continue
5315-
}
5316-
// Otherwise, we have a new store to begin tracking.
5317-
// Because we don't have type based alias analysis, we
5318-
// need to remove all other stores with the same offset
5319-
// before adding our own.
5320-
else {
5321-
prune_stores(&mut active_stores, offset);
5322-
active_stores.insert(key, insn_id);
5323-
}
5324-
}
5325-
// If a load is found, then any earlier store was
5326-
// necessary. We know this because redundant loads and
5327-
// stores were already eliminated, implying the
5328-
// hypothetical StoreField we haven't found yet has a
5329-
// different value than the one in active_stores.
5330-
// The following lines are required when we have type
5331-
// based alias analysis and the following call to prune
5332-
// stores is no longer used.
5333-
// Insn::LoadField { recv, offset, .. } => {
5334-
// let key = StoreHeap { object: self.chase_insn(recv), offset };
5335-
// active_stores.remove(&key);
5336-
Insn::LoadField{ offset, .. } => {
5337-
// Because we don't have type based alias analysis, we
5338-
// can't just remove the (up to) one entry of the
5339-
// StoreHeap key in active_stores. We need to remove all
5340-
// such keys that could alias.
5341-
prune_stores(&mut active_stores, offset);
5342-
}
5343-
Insn::WriteBarrier { .. } => {
5344-
// Currently, WriteBarrier write effects are Allocator and Memory when we'd really like them to be flags.
5345-
// We don't use LoadField for mark bits so we can ignore them for now.
5346-
// But flags does not exist in our effects abstract heap modeling and we don't want to add special casing to effects.
5347-
// This special casing in this pass here should be removed once we refine our effects system to provide greater granularity for WriteBarrier.
5348-
// TODO: use TBAA
5349-
let offset = RUBY_OFFSET_RBASIC_FLAGS;
5350-
prune_stores(&mut active_stores, offset);
5351-
},
5352-
insn => {
5353-
// If the instruction can read memory, we cannot assume
5354-
// entries of active_stores are not loaded. This is because we use
5355-
// extended basic blocks and we should probably change this to be
5356-
// standard basic blocks.
5357-
// TODO: Add a test for this case
5358-
if insn.effects_of().includes(Effect::read(abstract_heaps::Memory)) {
5359-
active_stores.clear();
5360-
}
5361-
else if insn.effects_of().includes(effects::Control) {
5362-
active_stores.clear();
5363-
}
5364-
}
5365-
}
5366-
new_insns.push(insn_id);
5367-
}
5368-
new_insns.reverse();
5369-
self.blocks[block.0].insns = new_insns;
5370-
}
5371-
}
5286+
// fn eliminate_dead_stores(&mut self) {
5287+
// #[derive(PartialEq, Eq, Hash)]
5288+
// struct StoreHeap {
5289+
// // TODO: Is object a good name here? If not, what should it be instead?
5290+
// object: InsnId,
5291+
// offset: i32,
5292+
// }
5293+
5294+
// // This helper function is used while we don't have type based alias
5295+
// // analysis. Matching offsets could result in aliased objects that need
5296+
// // to be clear.
5297+
// fn prune_stores(map: &mut HashMap<StoreHeap, InsnId>, offset: i32) {
5298+
// map.retain(|StoreHeap { object: _, offset: off }, _| *off != offset);
5299+
// }
5300+
5301+
// for block in self.rpo() {
5302+
// // TODO: Probably convert this to a vec of tuples? Time complexity is worse but constant factors are better
5303+
// let mut active_stores: HashMap<StoreHeap, InsnId> = HashMap::new();
5304+
// let insns = std::mem::take(&mut self.blocks[block.0].insns);
5305+
// let mut new_insns = vec![];
5306+
// for insn_id in insns.iter().rev().copied() {
5307+
// match self.find(insn_id) {
5308+
// Insn::StoreField { recv, offset, .. } => {
5309+
// let key = StoreHeap { object: self.chase_insn(recv), offset };
5310+
// // If an older store is being tracked, then there have
5311+
// // been no usees between stores and the first store is
5312+
// // dead and should not be emitted.
5313+
// if active_stores.contains_key(&key) {
5314+
// continue
5315+
// }
5316+
// // Otherwise, we have a new store to begin tracking.
5317+
// // Because we don't have type based alias analysis, we
5318+
// // need to remove all other stores with the same offset
5319+
// // before adding our own.
5320+
// else {
5321+
// prune_stores(&mut active_stores, offset);
5322+
// active_stores.insert(key, insn_id);
5323+
// }
5324+
// }
5325+
// // If a load is found, then any earlier store was
5326+
// // necessary. We know this because redundant loads and
5327+
// // stores were already eliminated, implying the
5328+
// // hypothetical StoreField we haven't found yet has a
5329+
// // different value than the one in active_stores.
5330+
// // The following lines are required when we have type
5331+
// // based alias analysis and the following call to prune
5332+
// // stores is no longer used.
5333+
// // Insn::LoadField { recv, offset, .. } => {
5334+
// // let key = StoreHeap { object: self.chase_insn(recv), offset };
5335+
// // active_stores.remove(&key);
5336+
// Insn::LoadField{ offset, .. } => {
5337+
// // Because we don't have type based alias analysis, we
5338+
// // can't just remove the (up to) one entry of the
5339+
// // StoreHeap key in active_stores. We need to remove all
5340+
// // such keys that could alias.
5341+
// prune_stores(&mut active_stores, offset);
5342+
// }
5343+
// Insn::WriteBarrier { .. } => {
5344+
// // Currently, WriteBarrier write effects are Allocator and Memory when we'd really like them to be flags.
5345+
// // We don't use LoadField for mark bits so we can ignore them for now.
5346+
// // But flags does not exist in our effects abstract heap modeling and we don't want to add special casing to effects.
5347+
// // This special casing in this pass here should be removed once we refine our effects system to provide greater granularity for WriteBarrier.
5348+
// // TODO: use TBAA
5349+
// let offset = RUBY_OFFSET_RBASIC_FLAGS;
5350+
// prune_stores(&mut active_stores, offset);
5351+
// },
5352+
// insn => {
5353+
// // If the instruction can read memory, we cannot assume
5354+
// // entries of active_stores are not loaded. This is because we use
5355+
// // extended basic blocks and we should probably change this to be
5356+
// // standard basic blocks.
5357+
// // TODO: Add a test for this case
5358+
// if insn.effects_of().includes(Effect::read(abstract_heaps::Memory)) {
5359+
// active_stores.clear();
5360+
// }
5361+
// else if insn.effects_of().includes(effects::Control) {
5362+
// active_stores.clear();
5363+
// }
5364+
// }
5365+
// }
5366+
// new_insns.push(insn_id);
5367+
// }
5368+
// new_insns.reverse();
5369+
// self.blocks[block.0].insns = new_insns;
5370+
// }
5371+
// }
53725372

53735373
/// Fold a binary operator on fixnums.
53745374
fn fold_fixnum_bop(&mut self, insn_id: InsnId, left: InsnId, right: InsnId, f: impl FnOnce(Option<i64>, Option<i64>) -> Option<i64>) -> InsnId {
@@ -6020,7 +6020,6 @@ impl Function {
60206020
(convert_no_profile_sends) => { Counter::compile_hir_strength_reduce_time_ns };
60216021
// End strength reduction bucket
60226022
(optimize_load_store) => { Counter::compile_hir_optimize_load_store_time_ns };
6023-
(eliminate_dead_stores) => { Counter::compile_hir_eliminate_dead_stores_time_ns };
60246023
(fold_constants) => { Counter::compile_hir_fold_constants_time_ns };
60256024
(clean_cfg) => { Counter::compile_hir_clean_cfg_time_ns };
60266025
(remove_redundant_patch_points) => { Counter::compile_hir_remove_redundant_patch_points_time_ns };
@@ -6053,7 +6052,6 @@ impl Function {
60536052
run_pass!(optimize_c_calls);
60546053
run_pass!(convert_no_profile_sends);
60556054
run_pass!(optimize_load_store);
6056-
// run_pass!(eliminate_dead_stores);
60576055
run_pass!(fold_constants);
60586056
run_pass!(clean_cfg);
60596057
run_pass!(remove_redundant_patch_points);

‎zjit/src/stats.rs‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,6 @@ make_counters! {
171171
compile_hir_build_time_ns,
172172
compile_hir_strength_reduce_time_ns,
173173
compile_hir_optimize_load_store_time_ns,
174-
compile_hir_eliminate_dead_stores_time_ns,
175174
compile_hir_fold_constants_time_ns,
176175
compile_hir_clean_cfg_time_ns,
177176
compile_hir_remove_redundant_patch_points_time_ns,

0 commit comments

Comments
 (0)