Skip to content

Commit

Permalink
PCC: rework default-facts somewhat. (#7262)
Browse files Browse the repository at this point in the history 
This removes the need for the awkward "max-range fact is subsumed by
anything" rule noted by @fitzgen in [this
comment](#7231 (comment)).
It also makes checking a little more efficient and logically clear, as
only the facts that the frontend/producer added are verified, rather
than all default facts as well.
  • Loading branch information
@cfallin
cfallin authored Oct 16, 2023
1 parent 99be9b0 commit 9e4d446
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 54 deletions.
7 changes: 2 additions & 5 deletions cranelift/codegen/src/ir/memtype.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,12 +160,9 @@ pub struct MemoryTypeField {
}

impl MemoryTypeField {
/// Get the fact, if any, on a field. Fills in a default inferred
/// fact based on the type if no explicit fact is present.
/// Get the fact, if any, on a field.
pub fn fact(&self) -> Option<&Fact> {
self.fact
.as_ref()
.or_else(|| Fact::infer_from_type(self.ty))
self.fact.as_ref()
}
}

Expand Down
11 changes: 1 addition & 10 deletions cranelift/codegen/src/ir/pcc.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -230,14 +230,6 @@ impl<'a> FactContext<'a> {
// Reflexivity.
(l, r) if l == r => true,

// Any value on the LHS subsumes a minimal (always-true)
// fact about the max value of a given bitwidth on the
// RHS: e.g., no matter the value, the bottom 8 bits will
// always be <= 255.
(_, Fact::ValueMax { bit_width, max }) if *max == max_value_for_width(*bit_width) => {
true
}

(
Fact::ValueMax {
bit_width: bw_lhs,
Expand Down Expand Up @@ -487,8 +479,7 @@ impl<'a> FactContext<'a> {
pub fn load<'b>(&'b self, fact: &Fact, access_ty: ir::Type) -> PccResult<Option<&'b Fact>> {
Ok(self
.struct_field(fact, access_ty)?
.and_then(|field| field.fact())
.or_else(|| Fact::infer_from_type(access_ty)))
.and_then(|field| field.fact()))
}

/// Check a store.
Expand Down
66 changes: 34 additions & 32 deletions cranelift/codegen/src/isa/aarch64/pcc.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
use crate::ir::pcc::*;
use crate::ir::types::*;
use crate::ir::MemFlags;
use crate::ir::Type;
use crate::isa::aarch64::inst::args::{PairAMode, ShiftOp};
use crate::isa::aarch64::inst::ALUOp;
use crate::isa::aarch64::inst::Inst;
Expand All @@ -11,8 +12,11 @@ use crate::machinst::Reg;
use crate::machinst::VCode;
use crate::trace;

fn get_fact(vcode: &VCode<Inst>, reg: Reg) -> PccResult<&Fact> {
vcode.vreg_fact(reg.into()).ok_or(PccError::MissingFact)
fn get_fact_or_default(vcode: &VCode<Inst>, reg: Reg) -> PccResult<&Fact> {
vcode
.vreg_fact(reg.into())
.or_else(|| Fact::infer_from_type(vcode.vreg_type(reg.into())))
.ok_or(PccError::MissingFact)
}

fn has_fact(vcode: &VCode<Inst>, reg: Reg) -> bool {
Expand Down Expand Up @@ -130,21 +134,19 @@ pub(crate) fn check(ctx: &FactContext, vcode: &VCode<Inst>, inst: &Inst) -> PccR
rd,
rn,
rm,
} if has_fact(vcode, *rn) && has_fact(vcode, *rm) => {
check_output(&ctx, vcode, rd.to_reg(), || {
let rn = get_fact(vcode, *rn)?;
let rm = get_fact(vcode, *rm)?;
fail_if_missing(ctx.add(rn, rm, size.bits().into()))
})
}
} => check_output(&ctx, vcode, rd.to_reg(), || {
let rn = get_fact_or_default(vcode, *rn)?;
let rm = get_fact_or_default(vcode, *rm)?;
fail_if_missing(ctx.add(rn, rm, size.bits().into()))
}),
Inst::AluRRImm12 {
alu_op: ALUOp::Add,
size,
rd,
rn,
imm12,
} if has_fact(vcode, *rn) => check_output(&ctx, vcode, rd.to_reg(), || {
let rn = get_fact(vcode, *rn)?;
} => check_output(&ctx, vcode, rd.to_reg(), || {
let rn = get_fact_or_default(vcode, *rn)?;
let imm_fact = Fact::ValueMax {
bit_width: size.bits().into(),
max: imm12.value(),
Expand All @@ -160,8 +162,8 @@ pub(crate) fn check(ctx: &FactContext, vcode: &VCode<Inst>, inst: &Inst) -> PccR
shiftop,
} if shiftop.op() == ShiftOp::LSL && has_fact(vcode, *rn) && has_fact(vcode, *rm) => {
check_output(&ctx, vcode, rd.to_reg(), || {
let rn = get_fact(vcode, *rn)?;
let rm = get_fact(vcode, *rm)?;
let rn = get_fact_or_default(vcode, *rn)?;
let rm = get_fact_or_default(vcode, *rm)?;
let rm_shifted = fail_if_missing(ctx.shl(
&rm,
size.bits().into(),
Expand All @@ -179,8 +181,8 @@ pub(crate) fn check(ctx: &FactContext, vcode: &VCode<Inst>, inst: &Inst) -> PccR
extendop,
} if has_fact(vcode, *rn) && has_fact(vcode, *rm) => {
check_output(&ctx, vcode, rd.to_reg(), || {
let rn = get_fact(vcode, *rn)?;
let rm = get_fact(vcode, *rm)?;
let rn = get_fact_or_default(vcode, *rn)?;
let rm = get_fact_or_default(vcode, *rm)?;
let rm_extended = fail_if_missing(extend_fact(&ctx, rm, *extendop))?;
fail_if_missing(ctx.add(&rn, &rm_extended, size.bits().into()))
})
Expand All @@ -193,7 +195,7 @@ pub(crate) fn check(ctx: &FactContext, vcode: &VCode<Inst>, inst: &Inst) -> PccR
immshift,
} if has_fact(vcode, *rn) && has_fact(vcode, *rn) => {
check_output(&ctx, vcode, rd.to_reg(), || {
let rn = get_fact(vcode, *rn)?;
let rn = get_fact_or_default(vcode, *rn)?;
fail_if_missing(ctx.shl(&rn, size.bits().into(), immshift.value().into()))
})
}
Expand All @@ -204,7 +206,7 @@ pub(crate) fn check(ctx: &FactContext, vcode: &VCode<Inst>, inst: &Inst) -> PccR
from_bits,
to_bits,
} if has_fact(vcode, *rn) => check_output(&ctx, vcode, rd.to_reg(), || {
let rn = get_fact(vcode, *rn)?;
let rn = get_fact_or_default(vcode, *rn)?;
fail_if_missing(ctx.uextend(&rn, (*from_bits).into(), (*to_bits).into()))
}),
Inst::AluRRR { size, rd, .. }
Expand Down Expand Up @@ -249,7 +251,7 @@ fn check_load(
vcode: &VCode<Inst>,
ty: Type,
) -> PccResult<()> {
let result_fact = rd.map(|rd| get_fact(vcode, rd)).transpose()?;
let result_fact = rd.and_then(|rd| vcode.vreg_fact(rd.into()));
check_addr(
ctx,
flags,
Expand All @@ -268,7 +270,7 @@ fn check_store(
vcode: &VCode<Inst>,
ty: Type,
) -> PccResult<()> {
let stored_fact = rd.map(|rd| get_fact(vcode, rd)).transpose()?;
let stored_fact = rd.and_then(|rd| vcode.vreg_fact(rd.into()));
check_addr(
ctx,
flags,
Expand Down Expand Up @@ -312,14 +314,14 @@ fn check_addr<'a>(

match addr {
&AMode::RegReg { rn, rm } => {
let rn = get_fact(vcode, rn)?;
let rm = get_fact(vcode, rm)?;
let rn = get_fact_or_default(vcode, rn)?;
let rm = get_fact_or_default(vcode, rm)?;
let sum = fail_if_missing(ctx.add(&rn, &rm, 64))?;
check(&sum, ty)
}
&AMode::RegScaled { rn, rm, ty } => {
let rn = get_fact(vcode, rn)?;
let rm = get_fact(vcode, rm)?;
let rn = get_fact_or_default(vcode, rn)?;
let rm = get_fact_or_default(vcode, rm)?;
let rm_scaled = fail_if_missing(ctx.scale(&rm, 64, ty.bytes()))?;
let sum = fail_if_missing(ctx.add(&rn, &rm_scaled, 64))?;
check(&sum, ty)
Expand All @@ -330,16 +332,16 @@ fn check_addr<'a>(
ty,
extendop,
} => {
let rn = get_fact(vcode, rn)?;
let rm = get_fact(vcode, rm)?;
let rn = get_fact_or_default(vcode, rn)?;
let rm = get_fact_or_default(vcode, rm)?;
let rm_extended = fail_if_missing(extend_fact(ctx, rm, extendop))?;
let rm_scaled = fail_if_missing(ctx.scale(&rm_extended, 64, ty.bytes()))?;
let sum = fail_if_missing(ctx.add(&rn, &rm_scaled, 64))?;
check(&sum, ty)
}
&AMode::RegExtended { rn, rm, extendop } => {
let rn = get_fact(vcode, rn)?;
let rm = get_fact(vcode, rm)?;
let rn = get_fact_or_default(vcode, rn)?;
let rm = get_fact_or_default(vcode, rm)?;
let rm_extended = fail_if_missing(extend_fact(ctx, rm, extendop))?;
let sum = fail_if_missing(ctx.add(&rn, &rm_extended, 64))?;
trace!("rn = {rn:?} rm = {rm:?} rm_extended = {rm_extended:?} sum = {sum:?}");
Expand All @@ -348,12 +350,12 @@ fn check_addr<'a>(
Ok(())
}
&AMode::Unscaled { rn, simm9 } => {
let rn = get_fact(vcode, rn)?;
let rn = get_fact_or_default(vcode, rn)?;
let sum = fail_if_missing(ctx.offset(&rn, 64, simm9.value.into()))?;
check(&sum, ty)
}
&AMode::UnsignedOffset { rn, uimm12 } => {
let rn = get_fact(vcode, rn)?;
let rn = get_fact_or_default(vcode, rn)?;
// Safety: this will not overflow: `size` should be at
// most 32 or 64 for large vector ops, and the `uimm12`'s
// value is at most 4095.
Expand All @@ -371,7 +373,7 @@ fn check_addr<'a>(
Ok(())
}
&AMode::RegOffset { rn, off, .. } => {
let rn = get_fact(vcode, rn)?;
let rn = get_fact_or_default(vcode, rn)?;
let sum = fail_if_missing(ctx.offset(&rn, 64, off))?;
check(&sum, ty)
}
Expand Down Expand Up @@ -417,7 +419,7 @@ fn check_load_addr(
if !flags.checked() {
return Ok(());
}
let fact = get_fact(vcode, reg)?;
let fact = get_fact_or_default(vcode, reg)?;
let _output_fact = ctx.load(fact, ty)?;
Ok(())
}
Expand All @@ -432,7 +434,7 @@ fn check_store_addr(
if !flags.checked() {
return Ok(());
}
let fact = get_fact(vcode, reg)?;
let fact = get_fact_or_default(vcode, reg)?;
let _output_fact = ctx.store(fact, ty, None)?;
Ok(())
}
13 changes: 6 additions & 7 deletions cranelift/codegen/src/machinst/vcode.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1287,16 +1287,15 @@ impl<I: VCodeInst> VCode<I> {
op
}

/// Get the type of a VReg.
pub fn vreg_type(&self, vreg: VReg) -> Type {
self.vreg_types[vreg.vreg()]
}

/// Get the fact, if any, for a given VReg.
pub fn vreg_fact(&self, vreg: VReg) -> Option<&Fact> {
let vreg = self.resolve_vreg_alias(vreg);
match &self.facts[vreg.vreg()] {
// The vreg has a stated fact -- return that.
Some(fact) => Some(fact),
// The vreg has no fact, but maybe we can infer a minimal
// one (e.g., an integer range) from the type.
None => Fact::infer_from_type(self.vreg_types[vreg.vreg()]),
}
self.facts[vreg.vreg()].as_ref()
}

/// Does a given instruction define any facts?
Expand Down

0 comments on commit 9e4d446

Please sign in to comment.