Skip to content

Commit

Permalink
cranelift: Parse iconst according to its type
Browse files Browse the repository at this point in the history
Modifies the parser for textual CLIF so that V in `iconst.T V` is
parsed according to T.

Before this commit, something like `iconst.i32 0xffff_ffff_ffff` was
valid because all `iconst` were parsed the same as an
`iconst.i64`. Now the above example will throw an error.

Also, a negative immediate as in `iconst.iN -X` is now converted to
`2^N - X`.

This commit also fixes some broken tests.
  • Loading branch information
timjrd committed Aug 29, 2023
1 parent cb3aaae commit 7f2358c
Show file tree
Hide file tree
Showing 7 changed files with 55 additions and 16 deletions.
14 changes: 12 additions & 2 deletions cranelift/codegen/src/legalizer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
use crate::cursor::{Cursor, FuncCursor};
use crate::flowgraph::ControlFlowGraph;
use crate::ir::immediates::Imm64;
use crate::ir::types::{I128, I64};
use crate::ir::types::{self, I128, I64};
use crate::ir::{self, InstBuilder, InstructionData, MemFlags, Value};
use crate::isa::TargetIsa;
use crate::trace;
Expand All @@ -38,7 +38,17 @@ fn imm_const(pos: &mut FuncCursor, arg: Value, imm: Imm64, is_signed: bool) -> V
let imm = pos.ins().iconst(I64, imm);
pos.ins().uextend(I128, imm)
}
_ => pos.ins().iconst(ty.lane_type(), imm),
_ => {
let bits = imm.bits();
let unsigned = match ty.lane_type() {
types::I8 => bits as u8 as i64,
types::I16 => bits as u16 as i64,
types::I32 => bits as u32 as i64,
types::I64 => bits,
_ => unreachable!(),
};
pos.ins().iconst(ty.lane_type(), unsigned)
}
}
}

Expand Down
13 changes: 12 additions & 1 deletion cranelift/codegen/src/write.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
use crate::entity::SecondaryMap;
use crate::ir::entities::AnyEntity;
use crate::ir::types;
use crate::ir::{Block, DataFlowGraph, Function, Inst, SigRef, Type, Value, ValueDef};
use crate::opts::Imm64;
use crate::packed_option::ReservedValue;
use alloc::string::{String, ToString};
use alloc::vec::Vec;
Expand Down Expand Up @@ -380,7 +382,16 @@ pub fn write_operands(w: &mut dyn Write, dfg: &DataFlowGraph, inst: Inst) -> fmt
LoadNoOffset { flags, arg, .. } => write!(w, "{} {}", flags, arg),
StoreNoOffset { flags, args, .. } => write!(w, "{} {}, {}", flags, args[0], args[1]),
Unary { arg, .. } => write!(w, " {}", arg),
UnaryImm { imm, .. } => write!(w, " {}", imm),
UnaryImm { imm, .. } => {
let bits = imm.bits();
let signed = match dfg.ctrl_typevar(inst) {
types::I8 => bits as i8 as i64,
types::I16 => bits as i16 as i64,
types::I32 => bits as i32 as i64,
_ => bits,
};
write!(w, " {}", Imm64::new(signed))
}
UnaryIeee32 { imm, .. } => write!(w, " {}", imm),
UnaryIeee64 { imm, .. } => write!(w, " {}", imm),
UnaryGlobalValue { global_value, .. } => write!(w, " {}", global_value),
Expand Down
2 changes: 1 addition & 1 deletion cranelift/filetests/filetests/egraph/extends.clif
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ target x86_64

function %f1() -> i64 {
block0:
v0 = iconst.i32 0xffff_ffff_9876_5432
v0 = iconst.i32 0x9876_5432
v1 = uextend.i64 v0
return v1
; check: v2 = iconst.i64 0x9876_5432
Expand Down
24 changes: 20 additions & 4 deletions cranelift/reader/src/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use cranelift_codegen::entity::{EntityRef, PrimaryMap};
use cranelift_codegen::ir::entities::{AnyEntity, DynamicType};
use cranelift_codegen::ir::immediates::{Ieee32, Ieee64, Imm64, Offset32, Uimm32, Uimm64};
use cranelift_codegen::ir::instructions::{InstructionData, InstructionFormat, VariableArgs};
use cranelift_codegen::ir::types;
use cranelift_codegen::ir::types::INVALID;
use cranelift_codegen::ir::types::*;
use cranelift_codegen::ir::{self, UserExternalNameRef};
Expand Down Expand Up @@ -2456,10 +2457,25 @@ impl<'a> Parser<'a> {
opcode,
arg: self.match_value("expected SSA value operand")?,
},
InstructionFormat::UnaryImm => InstructionData::UnaryImm {
opcode,
imm: self.match_imm64("expected immediate integer operand")?,
},
InstructionFormat::UnaryImm => {
let msg = |bits| format!("expected immediate {bits}-bit integer operand");
let unsigned = match explicit_control_type {
Some(types::I8) => self.match_imm8(&msg(8))? as u8 as i64,
Some(types::I16) => self.match_imm16(&msg(16))? as u16 as i64,
Some(types::I32) => self.match_imm32(&msg(32))? as u32 as i64,
Some(types::I64) => self.match_imm64(&msg(64))?.bits(),
_ => {
return err!(
self.loc,
"expected one of the following type: i8, i16, i32 or i64"
)
}
};
InstructionData::UnaryImm {
opcode,
imm: Imm64::new(unsigned),
}
}
InstructionFormat::UnaryIeee32 => InstructionData::UnaryIeee32 {
opcode,
imm: self.match_ieee32("expected immediate 32-bit float operand")?,
Expand Down
2 changes: 1 addition & 1 deletion cranelift/tests/bugpoint_test.clif
Original file line number Diff line number Diff line change
Expand Up @@ -910,7 +910,7 @@ block51:
v428 = iadd_imm.i64 v28, 8
v429 = load.i16 v428
v435 -> v429
v430 = iconst.i16 0xffff_ffff_ffff_8000
v430 = iconst.i16 0x8000
v431 = icmp eq v429, v430
v433 = uextend.i32 v431
brif v433, block154, block52
Expand Down
4 changes: 3 additions & 1 deletion cranelift/wasm/src/code_translator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -928,7 +928,9 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
translate_store(memarg, ir::Opcode::Store, builder, state, environ)?;
}
/****************************** Nullary Operators ************************************/
Operator::I32Const { value } => state.push1(builder.ins().iconst(I32, i64::from(*value))),
Operator::I32Const { value } => {
state.push1(builder.ins().iconst(I32, *value as u32 as i64))
}
Operator::I64Const { value } => state.push1(builder.ins().iconst(I64, *value)),
Operator::F32Const { value } => {
state.push1(builder.ins().f32const(f32_translation(*value)));
Expand Down
12 changes: 6 additions & 6 deletions cranelift/wasm/src/environ/dummy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -509,7 +509,7 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ
_heap: Heap,
_val: ir::Value,
) -> WasmResult<ir::Value> {
Ok(pos.ins().iconst(I32, -1))
Ok(pos.ins().iconst(I32, -1i32 as u32 as i64))
}

fn translate_memory_size(
Expand All @@ -518,7 +518,7 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ
_index: MemoryIndex,
_heap: Heap,
) -> WasmResult<ir::Value> {
Ok(pos.ins().iconst(I32, -1))
Ok(pos.ins().iconst(I32, -1i32 as u32 as i64))
}

fn translate_memory_copy(
Expand Down Expand Up @@ -570,7 +570,7 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ
_index: TableIndex,
_table: ir::Table,
) -> WasmResult<ir::Value> {
Ok(pos.ins().iconst(I32, -1))
Ok(pos.ins().iconst(I32, -1i32 as u32 as i64))
}

fn translate_table_grow(
Expand All @@ -581,7 +581,7 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ
_delta: ir::Value,
_init_value: ir::Value,
) -> WasmResult<ir::Value> {
Ok(pos.ins().iconst(I32, -1))
Ok(pos.ins().iconst(I32, -1i32 as u32 as i64))
}

fn translate_table_get(
Expand Down Expand Up @@ -660,7 +660,7 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ
mut pos: FuncCursor,
_global_index: GlobalIndex,
) -> WasmResult<ir::Value> {
Ok(pos.ins().iconst(I32, -1))
Ok(pos.ins().iconst(I32, -1i32 as u32 as i64))
}

fn translate_custom_global_set(
Expand All @@ -681,7 +681,7 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ
_expected: ir::Value,
_timeout: ir::Value,
) -> WasmResult<ir::Value> {
Ok(pos.ins().iconst(I32, -1))
Ok(pos.ins().iconst(I32, -1i32 as u32 as i64))
}

fn translate_atomic_notify(
Expand Down

0 comments on commit 7f2358c

Please sign in to comment.