Trim the MemFlags API

* Don't expose heap/table/vmctx flags, instead expose the `AliasRegion`
* Remove `tabletrap` accessors

cranelift/codegen/src/alias_analysis.rs

@@ -68,7 +68,7 @@ use crate::{
  inst_predicates::{
  has_memory_fence_semantics, inst_addr_offset_type, inst_store_data, visit_block_succs,
  },
- ir::{immediates::Offset32, Block, Function, Inst, Opcode, Type, Value},
+ ir::{immediates::Offset32, AliasRegion, Block, Function, Inst, Opcode, Type, Value},
  trace,
 };
 use cranelift_entity::{packed_option::PackedOption, EntityRef};
@@ -93,14 +93,11 @@ impl LastStores {
  self.other = inst.into();
  } else if opcode.can_store() {
  if let Some(memflags) = func.dfg.insts[inst].memflags() {
- if memflags.heap() {
- self.heap = inst.into();
- } else if memflags.table() {
- self.table = inst.into();
- } else if memflags.vmctx() {
- self.vmctx = inst.into();
- } else {
- self.other = inst.into();
+ match memflags.alias_region() {
+ None => self.other = inst.into(),
+ Some(AliasRegion::Heap) => self.heap = inst.into(),
+ Some(AliasRegion::Table) => self.table = inst.into(),
+ Some(AliasRegion::Vmctx) => self.vmctx = inst.into(),
  }
  } else {
  self.heap = inst.into();
@@ -113,14 +110,11 @@ impl LastStores {
 
  fn get_last_store(&self, func: &Function, inst: Inst) -> PackedOption<Inst> {
  if let Some(memflags) = func.dfg.insts[inst].memflags() {
- if memflags.heap() {
- self.heap
- } else if memflags.table() {
- self.table
- } else if memflags.vmctx() {
- self.vmctx
- } else {
- self.other
+ match memflags.alias_region() {
+ None => self.other,
+ Some(AliasRegion::Heap) => self.heap,
+ Some(AliasRegion::Table) => self.table,
+ Some(AliasRegion::Vmctx) => self.vmctx,
  }
  } else if func.dfg.insts[inst].opcode().can_load()
  || func.dfg.insts[inst].opcode().can_store()

cranelift/codegen/src/ir/memflags.rs

@@ -69,6 +69,8 @@ pub enum AliasRegion {
 #[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)]
 #[cfg_attr(feature = "enable-serde", derive(Serialize, Deserialize))]
 pub struct MemFlags {
+ // Initialized to all zeros to have all flags have their default value.
+ // This is the interpreted through various methods below.
  bits: u16,
 }
 
@@ -95,8 +97,9 @@ impl MemFlags {
  self
  }
 
- /// TODO
- const fn alias_region(self) -> Option<AliasRegion> {
+ /// Reads the alias region that this memory operation works with.
+ pub const fn alias_region(self) -> Option<AliasRegion> {
+ // NB: keep in sync with `with_alias_region`
  match (self.bits & MASK_ALIAS_REGION) >> ALIAS_REGION_OFFSET {
  0b00 => None,
  0b01 => Some(AliasRegion::Heap),
@@ -106,8 +109,9 @@ impl MemFlags {
  }
  }
 
- /// TODO
- const fn with_alias_region(mut self, region: Option<AliasRegion>) -> Self {
+ /// Sets the alias region that this works on to the specified `region`.
+ pub const fn with_alias_region(mut self, region: Option<AliasRegion>) -> Self {
+ // NB: keep in sync with `alias_region`
  let bits = match region {
  None => 0b00,
  Some(AliasRegion::Heap) => 0b01,
@@ -119,11 +123,21 @@ impl MemFlags {
  self
  }
 
+ /// Sets the alias region that this works on to the specified `region`.
+ pub fn set_alias_region(&mut self, region: Option<AliasRegion>) {
+ *self = self.with_alias_region(region);
+ }
+
  /// Set a flag bit by name.
  ///
  /// Returns true if the flag was found and set, false for an unknown flag
  /// name. Will also return false when trying to set inconsistent endianness
  /// flags.
+ ///
+ /// # Errors
+ ///
+ /// Returns an error message if the `name` is known but couldn't be applied
+ /// due to it being a semantic error.
  pub fn set_by_name(&mut self, name: &str) -> Result<bool, &'static str> {
  *self = match name {
  "notrap" => self.with_trap_code(None),
@@ -201,48 +215,31 @@ impl MemFlags {
  res
  }
 
- /// Test if the `notrap` flag is set.
+ /// Test if this memory operation cannot trap.
  ///
- /// Normally, trapping is part of the semantics of a load/store operation. If the platform
- /// would cause a trap when accessing the effective address, the Cranelift memory operation is
- /// also required to trap.
+ /// By default `MemFlags` will assume that any load/store can trap and is
+ /// associated with a `TrapCode::HeapOutOfBounds` code. If the trap code is
+ /// configured to `None` though then this method will return `true` and
+ /// indicates that the memory operation will not trap.
  ///
- /// The `notrap` flag tells Cranelift that the memory is *accessible*, which means that
- /// accesses will not trap. This makes it possible to delete an unused load or a dead store
- /// instruction.
+ /// If this returns `true` then the memory is *accessible*, which means
+ /// that accesses will not trap. This makes it possible to delete an unused
+ /// load or a dead store instruction.
  pub const fn notrap(self) -> bool {
  self.trap_code().is_none()
  }
 
- /// Set the `notrap` flag.
+ /// Sets the trap code for this `MemFlags` to `None`.
  pub fn set_notrap(&mut self) {
  *self = self.with_notrap();
  }
 
- /// Set the `notrap` flag, returning new flags.
+ /// Sets the trap code for this `MemFlags` to `None`, returning the new
+ /// flags.
  pub const fn with_notrap(self) -> Self {
  self.with_trap_code(None)
  }
 
- /// Test if the `tabletrap` bit is set.
- ///
- /// By default, if this memory access traps, the associated trap
- /// code will be `HeapOutOfBounds`. With this flag set, the trap
- /// code will instead be `TableOutOfBounds`.
- pub const fn tabletrap(self) -> bool {
- matches!(self.trap_code(), Some(TrapCode::TableOutOfBounds))
- }
-
- /// Set the `tabletrap` bit.
- pub fn set_tabletrap(&mut self) {
- *self = self.with_tabletrap();
- }
-
- /// Set the `tabletrap` bit, returning new flags.
- pub const fn with_tabletrap(self) -> Self {
- self.with_trap_code(Some(TrapCode::TableOutOfBounds))
- }
-
  /// Test if the `aligned` flag is set.
  ///
  /// By default, Cranelift memory instructions work with any unaligned effective address. If the
@@ -281,78 +278,6 @@ impl MemFlags {
  self.with_bit(BIT_READONLY)
  }
 
- /// Test if the `heap` bit is set.
- ///
- /// Loads and stores with this flag accesses the "heap" part of
- /// abstract state. This is disjoint from the "table", "vmctx",
- /// and "other" parts of abstract state. In concrete terms, this
- /// means that behavior is undefined if the same memory is also
- /// accessed by another load/store with one of the other
- /// alias-analysis bits (`table`, `vmctx`) set, or `heap` not set.
- pub const fn heap(self) -> bool {
- matches!(self.alias_region(), Some(AliasRegion::Heap))
- }
-
- /// Set the `heap` bit. See the notes about mutual exclusion with
- /// other bits in `heap()`.
- pub fn set_heap(&mut self) {
- *self = self.with_heap();
- }
-
- /// Set the `heap` bit, returning new flags.
- pub const fn with_heap(self) -> Self {
- assert!(self.alias_region().is_none());
- self.with_alias_region(Some(AliasRegion::Heap))
- }
-
- /// Test if the `table` bit is set.
- ///
- /// Loads and stores with this flag accesses the "table" part of
- /// abstract state. This is disjoint from the "heap", "vmctx",
- /// and "other" parts of abstract state. In concrete terms, this
- /// means that behavior is undefined if the same memory is also
- /// accessed by another load/store with one of the other
- /// alias-analysis bits (`heap`, `vmctx`) set, or `table` not set.
- pub const fn table(self) -> bool {
- matches!(self.alias_region(), Some(AliasRegion::Table))
- }
-
- /// Set the `table` bit. See the notes about mutual exclusion with
- /// other bits in `table()`.
- pub fn set_table(&mut self) {
- *self = self.with_table();
- }
-
- /// Set the `table` bit, returning new flags.
- pub const fn with_table(self) -> Self {
- assert!(self.alias_region().is_none());
- self.with_alias_region(Some(AliasRegion::Table))
- }
-
- /// Test if the `vmctx` bit is set.
- ///
- /// Loads and stores with this flag accesses the "vmctx" part of
- /// abstract state. This is disjoint from the "heap", "table",
- /// and "other" parts of abstract state. In concrete terms, this
- /// means that behavior is undefined if the same memory is also
- /// accessed by another load/store with one of the other
- /// alias-analysis bits (`heap`, `table`) set, or `vmctx` not set.
- pub const fn vmctx(self) -> bool {
- matches!(self.alias_region(), Some(AliasRegion::Vmctx))
- }
-
- /// Set the `vmctx` bit. See the notes about mutual exclusion with
- /// other bits in `vmctx()`.
- pub fn set_vmctx(&mut self) {
- *self = self.with_vmctx();
- }
-
- /// Set the `vmctx` bit, returning new flags.
- pub const fn with_vmctx(self) -> Self {
- assert!(self.alias_region().is_none());
- self.with_alias_region(Some(AliasRegion::Vmctx))
- }
-
  /// Test if the `checked` bit is set.
  ///
  /// Loads and stores with this flag are verified to access
@@ -378,11 +303,9 @@ impl MemFlags {
  self.with_bit(BIT_CHECKED)
  }
 
- /// Get the trap code to report if this memory access traps. If the
- /// `notrap` flag is set, then return `None`. Otherwise, the choice
- /// of trap code depends on the `tabletrap` flag. The default trap
- /// code is `HeapOutOfBounds`, but with `tabletrap` set, the trap
- /// code is instead `TableOutOfBounds`.
+ /// Get the trap code to report if this memory access traps.
+ ///
+ /// A `None` trap code indicates that this memory access does not trap.
  pub const fn trap_code(self) -> Option<TrapCode> {
  match (self.bits & MASK_TRAP_CODE) >> TRAP_CODE_OFFSET {
  0b0000 => Some(TrapCode::HeapOutOfBounds), // default mode
@@ -405,7 +328,9 @@ impl MemFlags {
  }
  }
 
- /// TODO
+ /// Configures these flags with the specified trap code `code`.
+ ///
+ /// Note that `TrapCode::User(_)` cannot be set in `MemFlags`.
  pub const fn with_trap_code(mut self, code: Option<TrapCode>) -> Self {
  let bits = match code {
  Some(TrapCode::StackOverflow) => 0b0001,

cranelift/codegen/src/ir/mod.rs

@@ -53,7 +53,7 @@ pub use crate::ir::jumptable::JumpTableData;
 pub use crate::ir::known_symbol::KnownSymbol;
 pub use crate::ir::layout::Layout;
 pub use crate::ir::libcall::{get_probestack_funcref, LibCall};
-pub use crate::ir::memflags::{Endianness, MemFlags};
+pub use crate::ir::memflags::{AliasRegion, Endianness, MemFlags};
 pub use crate::ir::memtype::{MemoryTypeData, MemoryTypeField};
 pub use crate::ir::pcc::{BaseExpr, Expr, Fact, FactContext, PccError, PccResult};
 pub use crate::ir::progpoint::ProgramPoint;

cranelift/fuzzgen/src/function_generator.rs

@@ -9,8 +9,8 @@ use cranelift::codegen::ir::instructions::{InstructionFormat, ResolvedConstraint
 use cranelift::codegen::ir::stackslot::StackSize;
 
 use cranelift::codegen::ir::{
- types::*, AtomicRmwOp, Block, ConstantData, Endianness, ExternalName, FuncRef, Function,
- LibCall, Opcode, SigRef, Signature, StackSlot, UserExternalName, UserFuncName, Value,
+ types::*, AliasRegion, AtomicRmwOp, Block, ConstantData, Endianness, ExternalName, FuncRef,
+ Function, LibCall, Opcode, SigRef, Signature, StackSlot, UserExternalName, UserFuncName, Value,
 };
 use cranelift::codegen::isa::CallConv;
 use cranelift::frontend::{FunctionBuilder, FunctionBuilderContext, Switch, Variable};
@@ -1176,12 +1176,12 @@ impl AACategory {
  }
 
  pub fn update_memflags(&self, flags: &mut MemFlags) {
- match self {
- AACategory::Other => {}
- AACategory::Heap => flags.set_heap(),
- AACategory::Table => flags.set_table(),
- AACategory::VmCtx => flags.set_vmctx(),
- }
+ flags.set_alias_region(match self {
+ AACategory::Other => None,
+ AACategory::Heap => Some(AliasRegion::Heap),
+ AACategory::Table => Some(AliasRegion::Table),
+ AACategory::VmCtx => Some(AliasRegion::Vmctx),
+ })
  }
 }
 

cranelift/wasm/src/code_translator.rs

@@ -177,7 +177,7 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
  let addr = builder.ins().global_value(environ.pointer_type(), gv);
  let mut flags = ir::MemFlags::trusted();
  // Put globals in the "table" abstract heap category as well.
- flags.set_table();
+ flags.set_alias_region(Some(ir::AliasRegion::Table));
  builder.ins().load(ty, flags, addr, offset)
  }
  GlobalVariable::Custom => environ.translate_custom_global_get(
@@ -194,7 +194,7 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
  let addr = builder.ins().global_value(environ.pointer_type(), gv);
  let mut flags = ir::MemFlags::trusted();
  // Put globals in the "table" abstract heap category as well.
- flags.set_table();
+ flags.set_alias_region(Some(ir::AliasRegion::Table));
  let mut val = state.pop1();
  // Ensure SIMD values are cast to their default Cranelift type, I8x16.
  if ty.is_vector() {
@@ -2837,7 +2837,7 @@ where
  // state. This may allow alias analysis to merge redundant loads,
  // etc. when heap accesses occur interleaved with other (table,
  // vmctx, stack) accesses.
- flags.set_heap();
+ flags.set_alias_region(Some(ir::AliasRegion::Heap));
 
  Ok(Reachability::Reachable((flags, index, addr)))
 }

cranelift/wasm/src/environ/dummy.rs

@@ -613,7 +613,7 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ
  self.ensure_table_exists(builder.func, table_index);
  let table = self.tables[table_index].as_ref().unwrap();
  let table_entry_addr = table.prepare_table_addr(builder, index, pointer_type, true);
- let flags = ir::MemFlags::trusted().with_table();
+ let flags = ir::MemFlags::trusted().with_alias_region(Some(ir::AliasRegion::Table));
  let value = builder
  .ins()
  .load(self.reference_type(), flags, table_entry_addr, 0);
@@ -631,7 +631,7 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ
  self.ensure_table_exists(builder.func, table_index);
  let table = self.tables[table_index].as_ref().unwrap();
  let table_entry_addr = table.prepare_table_addr(builder, index, pointer_type, true);
- let flags = ir::MemFlags::trusted().with_table();
+ let flags = ir::MemFlags::trusted().with_alias_region(Some(ir::AliasRegion::Table));
  builder.ins().store(flags, value, table_entry_addr, 0);
  Ok(())
  }