From 1f0e31da9a924a6b644d6fa5d4693450ed8fb193 Mon Sep 17 00:00:00 2001 From: Laurence Tratt Date: Mon, 4 Nov 2024 12:00:05 +0000 Subject: [PATCH 1/3] This is an "OPT" not a "FIXME". --- ykrt/src/compile/jitc_yk/opt/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ykrt/src/compile/jitc_yk/opt/mod.rs b/ykrt/src/compile/jitc_yk/opt/mod.rs index 44e68ee60..52446ec3e 100644 --- a/ykrt/src/compile/jitc_yk/opt/mod.rs +++ b/ykrt/src/compile/jitc_yk/opt/mod.rs @@ -379,7 +379,7 @@ impl Opt { if let Inst::Tombstone = inst { return; } - // FIXME: This is O(n), but most instructions can't possibly be CSE candidates. + // OPT: This is O(n), but most instructions can't possibly be CSE candidates. for back_iidx in (0..usize::from(iidx)).rev() { let back_iidx = InstIdx::unchecked_from(back_iidx); // Only examine non-`Copy` instructions, to avoid us continually checking the same From 01a1166eaa4677a2e475ed371a2b6efcc4acb397 Mon Sep 17 00:00:00 2001 From: Laurence Tratt Date: Mon, 4 Nov 2024 12:02:37 +0000 Subject: [PATCH 2/3] Hoist loop-invariant guard. I'm not quite sure how I didn't notice this earlier! --- ykrt/src/compile/jitc_yk/opt/mod.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/ykrt/src/compile/jitc_yk/opt/mod.rs b/ykrt/src/compile/jitc_yk/opt/mod.rs index 52446ec3e..0aa7bdff4 100644 --- a/ykrt/src/compile/jitc_yk/opt/mod.rs +++ b/ykrt/src/compile/jitc_yk/opt/mod.rs @@ -379,6 +379,14 @@ impl Opt { if let Inst::Tombstone = inst { return; } + // We don't perform CSE on instructions that have / enforce effects. + if inst.has_store_effect(&self.m) + || inst.has_load_effect(&self.m) + || inst.is_barrier(&self.m) + { + return; + } + // OPT: This is O(n), but most instructions can't possibly be CSE candidates. for back_iidx in (0..usize::from(iidx)).rev() { let back_iidx = InstIdx::unchecked_from(back_iidx); @@ -387,11 +395,8 @@ impl Opt { let Some(back) = self.m.inst_nocopy(back_iidx) else { continue; }; - if !inst.has_store_effect(&self.m) - && !inst.has_load_effect(&self.m) - && !inst.is_barrier(&self.m) - && inst.decopy_eq(&self.m, back) - { + + if inst.decopy_eq(&self.m, back) { self.m.replace(iidx, Inst::Copy(back_iidx)); return; } From 3e9cc9c1f0bcad60f12bd98d8341b257b6e1da3c Mon Sep 17 00:00:00 2001 From: Laurence Tratt Date: Mon, 4 Nov 2024 12:07:37 +0000 Subject: [PATCH 3/3] Optimise repeated guards. If two guards both reference the same variable, the second guard is redundant and can be removed. This optimisation isn't just a luxury: because our codegen optimises ICmp followed by a guard, repeated guards can reference a variable which we haven't materialised as a run-time value (other than via a temporary CPU flag). In other words, if we don't remove repeated guards, we can run into a "register allocator can't find the variable you're referring to" error. --- ykrt/src/compile/jitc_yk/opt/mod.rs | 45 ++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/ykrt/src/compile/jitc_yk/opt/mod.rs b/ykrt/src/compile/jitc_yk/opt/mod.rs index 0aa7bdff4..1e7c4a7f0 100644 --- a/ykrt/src/compile/jitc_yk/opt/mod.rs +++ b/ykrt/src/compile/jitc_yk/opt/mod.rs @@ -298,7 +298,27 @@ impl Opt { // doesn't affect future analyses. self.m.replace(iidx, Inst::Tombstone); } else { - self.an.guard(&self.m, x); + // Remove guards that reference the same i1 variable: if the earlier guard + // succeeded, then this guard must also succeed, by definition. + let mut removed = false; + // OPT: This is O(n), but most instructions can't possibly be guards. + for back_iidx in (0..usize::from(iidx)).rev() { + let back_iidx = InstIdx::unchecked_from(back_iidx); + // Only examine non-`Copy` instructions, to avoid us continually checking the same + // (subset of) instructions over and over again. + let Some(Inst::Guard(y)) = self.m.inst_nocopy(back_iidx) else { + continue; + }; + if x.cond(&self.m) == y.cond(&self.m) { + debug_assert_eq!(x.expect, y.expect); + self.m.replace(iidx, Inst::Tombstone); + removed = true; + break; + } + } + if !removed { + self.an.guard(&self.m, x); + } } } Inst::PtrAdd(x) => match self.an.op_map(&self.m, x.ptr(&self.m)) { @@ -979,4 +999,27 @@ mod test { ", ); } + + #[test] + fn opt_guard_dup() { + Module::assert_ir_transform_eq( + " + entry: + %0: i8 = load_ti 0 + %1: i8 = load_ti 1 + %2: i1 = eq %0, %1 + guard true, %2, [%0, %1] + guard true, %2, [%0, %1] + ", + |m| opt(m).unwrap(), + " + ... + entry: + %0: i8 = load_ti ... + %1: i8 = load_ti ... + %2: i1 = eq %0, %1 + guard true, %2, ... + ", + ); + } }