For discussion #1
Conversation
| ) | ||
| .operands_in(vec![a]) | ||
| .operands_out(vec![a]), | ||
| ); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yes indeed. This should also allow to transform pshuf with specific immediates into smaller / faster sequences of instructions, in a (future) pipehole optimizer.
| r#" | ||
| {{PUT_OP}}(bits, rex2(in_reg0, out_reg0), sink); | ||
| modrm_rr(in_reg0, out_reg0, sink); | ||
| sink.put1(0); |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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...)
| // def!(a = bitcast(x)), | ||
| // def!(y = x86_pshuf(a)) | ||
| // ] | ||
| // ); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
Thanks for taking a look; I'll continue in #3 |
Minor cleanups: Use vec![] instead of iterator
@bnjbvr, I don't mean this a real request to merge but rather to make it easier to discuss what I'm working on until your branch is merged.