Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

For discussion #1

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions cranelift-codegen/meta/src/cdsl/instructions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use crate::cdsl::formats::{
};
use crate::cdsl::operands::Operand;
use crate::cdsl::type_inference::Constraint;
use crate::cdsl::types::{LaneType, ValueType};
use crate::cdsl::types::{LaneType, ValueType, VectorType};
use crate::cdsl::typevar::TypeVar;

#[derive(Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
Expand Down Expand Up @@ -188,6 +188,14 @@ impl Instruction {
pub fn bind(&self, lane_type: impl Into<LaneType>) -> BoundInstruction {
bind(self.clone(), Some(lane_type.into()), Vec::new())
}

pub fn bind_vector(&self, lane_type: impl Into<LaneType>, num_lanes: u64) -> BoundInstruction {
abrown marked this conversation as resolved.
Show resolved Hide resolved
let vector_type = ValueType::Vector(VectorType::new(lane_type.into(), num_lanes));
let value_types = vec![ValueTypeOrAny::ValueType(vector_type)];
verify_polymorphic_binding(self, &value_types);
BoundInstruction { inst: self.clone(), value_types }
}

pub fn bind_any(&self) -> BoundInstruction {
bind(self.clone(), None, Vec::new())
}
Expand Down Expand Up @@ -1078,6 +1086,16 @@ fn bind(
}
}

verify_polymorphic_binding(&inst, &value_types);

BoundInstruction { inst, value_types }
}

/// Helper to verify that binding types to the instruction does not violate polymorphic rules
fn verify_polymorphic_binding(
inst: &Instruction,
value_types: &Vec<ValueTypeOrAny>,
) {
abrown marked this conversation as resolved.
Show resolved Hide resolved
match &inst.polymorphic_info {
Some(poly) => {
assert!(
Expand All @@ -1092,6 +1110,4 @@ fn bind(
));
}
}

BoundInstruction { inst, value_types }
}
5 changes: 5 additions & 0 deletions cranelift-codegen/meta/src/isa/x86/encodings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,7 @@ pub fn define(
let x86_fmax = x86.by_name("x86_fmax");
let x86_fmin = x86.by_name("x86_fmin");
let x86_pop = x86.by_name("x86_pop");
let x86_pshuf = x86.by_name("x86_pshuf");
let x86_push = x86.by_name("x86_push");
let x86_sdivmodx = x86.by_name("x86_sdivmodx");
let x86_smulx = x86.by_name("x86_smulx");
Expand Down Expand Up @@ -520,6 +521,10 @@ pub fn define(
// Definitions.
let mut e = PerCpuModeEncodings::new();

// x86 requires a data move to the XMM registers which is part of legalize.rs; this simply implements the shuffling of bits
e.enc32(x86_pshuf.bind_vector(I32, 4), r.template("splat").nonrex().opcodes(vec![0x66, 0x0f, 0x70]));
e.enc64(x86_pshuf.bind_vector(I32, 4), r.template("splat").nonrex().opcodes(vec![0x66, 0x0f, 0x70]));

e.enc_i32_i64(iadd, rec_rr.opcodes(vec![0x01]));
e.enc_i32_i64(isub, rec_rr.opcodes(vec![0x29]));
e.enc_i32_i64(band, rec_rr.opcodes(vec![0x21]));
Expand Down
27 changes: 27 additions & 0 deletions cranelift-codegen/meta/src/isa/x86/instructions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -249,5 +249,32 @@ pub fn define(
.operands_out(vec![y, rflags]),
);

let TxN = &TypeVar::new(
"TxN",
"A SIMD vector type",
TypeSetBuilder::new()
.ints(Interval::All)
.floats(Interval::All)
.bools(Interval::All)
.simd_lanes(Interval::All)
.includes_scalars(false)
.build(),
);
let a = &operand("a", TxN);
//let b = &operand("b", TxN);
ig.push(
Inst::new(
"x86_pshuf",
r#"
Shuffle Packed -- copies data from either memory or lanes in an extended
register and re-orders the data according to the passed immediate byte.
TODO allow memory type here
TODO how to allow passing in immediate during code generation?
"#,
)
.operands_in(vec![a])
.operands_out(vec![a]),
);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it would be most transparent to future engineers if I just implement the x86 instruction itself so they know what is intended; however, for this to be truly flexible I would like to be able to pass in the u8 immediate like x86_pshuf.bind_vector(I32, 4).set_immediate(0x...) in the encodings. Obviously set_immediate does not exist but I see a rrr used to pass in constants to the template; is that only for register addressing?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind, I think I see how to do this: add a new operand here based on teh immediates.by_name("uimm8") and then in legalize.rs do something like:

    let shuffle_encoding = Literal::constant(uimm8, 0x00);
    group.legalize(
        def!(y = splat(x)),
        vec![
            def!(a = bitcast(x)),
            def!(y = x86_pshuf(a, shuffle_encoding))
        ]
    );

Does that sound right?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes indeed. This should also allow to transform pshuf with specific immediates into smaller / faster sequences of instructions, in a (future) pipehole optimizer.


ig.build()
}
12 changes: 12 additions & 0 deletions cranelift-codegen/meta/src/isa/x86/legalize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ pub fn define(shared: &mut SharedDefinitions, x86_instructions: &InstructionGrou
// List of instructions.
let insts = &shared.instructions;
let band = insts.by_name("band");
let bitcast = insts.by_name("bitcast");
let bor = insts.by_name("bor");
let clz = insts.by_name("clz");
let ctz = insts.by_name("ctz");
Expand All @@ -38,6 +39,7 @@ pub fn define(shared: &mut SharedDefinitions, x86_instructions: &InstructionGrou
let sdiv = insts.by_name("sdiv");
let selectif = insts.by_name("selectif");
let smulhi = insts.by_name("smulhi");
let splat = insts.by_name("splat");
let srem = insts.by_name("srem");
let udiv = insts.by_name("udiv");
let umulhi = insts.by_name("umulhi");
Expand All @@ -46,6 +48,7 @@ pub fn define(shared: &mut SharedDefinitions, x86_instructions: &InstructionGrou

let x86_bsf = x86_instructions.by_name("x86_bsf");
let x86_bsr = x86_instructions.by_name("x86_bsr");
let x86_pshuf = x86_instructions.by_name("x86_pshuf");
let x86_umulx = x86_instructions.by_name("x86_umulx");
let x86_smulx = x86_instructions.by_name("x86_smulx");

Expand Down Expand Up @@ -82,6 +85,15 @@ pub fn define(shared: &mut SharedDefinitions, x86_instructions: &InstructionGrou
vec![def!((res_lo, res_hi) = x86_smulx(x, y))],
);

// SIMD
// group.legalize(
// def!(y = splat(x)),
// vec![
// def!(a = bitcast(x)),
// def!(y = x86_pshuf(a))
// ]
// );
Copy link
Author

@abrown abrown Jul 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a sticking point for me: if I uncomment this the code generation will fail:

$ CRANELIFT_VERBOSE=1 cargo build && target/debug/clif-util test -v filetests/isa/x86/simd.clif 
   Compiling cranelift-codegen-meta v0.30.0 (/home/abrown/Code/cranelift/cranelift-codegen/meta)
   Compiling cranelift-codegen v0.30.0 (/home/abrown/Code/cranelift/cranelift-codegen)
error: failed to run custom build command for `cranelift-codegen v0.30.0 (/home/abrown/Code/cranelift/cranelift-codegen)`

Caused by:
  process didn't exit successfully: `/home/abrown/Code/cranelift/target/debug/build/cranelift-codegen-1f59b278cc90c54e/build-script-build` (exit code: 101)
--- stdout
cargo:rerun-if-changed=/home/abrown/Code/cranelift/cranelift-codegen/build.rs

--- stderr
thread 'main' panicked at 'assertion failed: vars_tv.contains(arg)', cranelift-codegen/meta/src/cdsl/type_inference.rs:367:17
note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

The real issue is that I don't really understand how the type system which I observe in encoding.rs, e.g. I32s and my addition of bind_vector, ties in with this error. Should I be specifying the types of these variables more explicitly?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the variables a, x, and y refer to variables that might be typed with a type variable; since the build error happens during type inference, that might be it. The error message is very cryptic and could probably be enhanced in any case. I am going to try this locally.


// Floating point condition codes.
//
// The 8 condition codes in `supported_floatccs` are directly supported by a
Expand Down
15 changes: 15 additions & 0 deletions cranelift-codegen/meta/src/isa/x86/recipes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,21 @@ pub fn define<'shared>(
),
);

{
recipes.add_template_recipe(
EncodingRecipeBuilder::new("splat", f_unary, 2)
.operands_in(vec![fpr])
.operands_out(vec![fpr])
.emit(
r#"
{{PUT_OP}}(bits, rex2(in_reg0, out_reg0), sink);
modrm_rr(in_reg0, out_reg0, sink);
sink.put1(0);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll rename this recipe to something else... but what? And the sink.put1(0) is a hardcoding that would be nice to avoid (see comments on pshuf above)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this can be reused for all the pshufw/d etc., maybe the name could be pshuf_base or something like this? (Or if you understand the preexistent naming scheme with rr etc, apply the scheme here too? I'm not too sure how this naming scheme works...)

"#,
),
);
}

// XX /r with operands swapped. (RM form).
recipes.add_template_recipe(
EncodingRecipeBuilder::new("rrx", f_binary, 1)
Expand Down
23 changes: 23 additions & 0 deletions filetests/isa/x86/simd.clif
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
test binemit
; set opt_level=best
target x86_64

; The binary encodings can be verified with the command:
;
; sed -ne 's/^ *; asm: *//p' filetests/isa/x86/shrink.clif | llvm-mc -show-encoding -triple=x86_64
;

; run with `target/debug/clif-util test -v filetests/isa/x86/simd.clif`
;function %test_simple() {
;ebb0:
; [RexOp1pu_id#b8, %rax] v1 = iconst.i32 42 ; bin: 40 b8 0000002a
; [Op1pu_id#b8, %rax] v2 = iconst.i32 42 ; bin: b8 0000002a
; [Op1ret#c3] return ; bin: c3
;}

function %test_simd() {
ebb0:
[-, %rax] v0 = iconst.i32 42
[-, %xmm0] v1 = splat.i32x4 v0 ; bin: 66 0f 70 c0 00
return
}