Skip to content

Commit acbb8d4

Browse files
rwstaunerXrXr
rwstauner
authored and
XrXr
committed
Expand opt_newarray_send to support Array#pack with buffer keyword arg
Use an enum for the method arg instead of needing to add an id that doesn't map to an actual method name. $ ruby --dump=insns -e 'b = "x"; [v].pack("E*", buffer: b)' before: ``` == disasm: #<ISeq:<main>@-e:1 (1,0)-(1,34)> local table (size: 1, argc: 0 [opts: 0, rest: -1, post: 0, block: -1, kw: -1@-1, kwrest: -1]) [ 1] b@0 0000 putchilledstring "x" ( 1)[Li] 0002 setlocal_WC_0 b@0 0004 putself 0005 opt_send_without_block <calldata!mid:v, argc:0, FCALL|VCALL|ARGS_SIMPLE> 0007 newarray 1 0009 putchilledstring "E*" 0011 getlocal_WC_0 b@0 0013 opt_send_without_block <calldata!mid:pack, argc:2, kw:[#<Symbol:0x000000000023110c>], KWARG> 0015 leave ``` after: ``` == disasm: #<ISeq:<main>@-e:1 (1,0)-(1,34)> local table (size: 1, argc: 0 [opts: 0, rest: -1, post: 0, block: -1, kw: -1@-1, kwrest: -1]) [ 1] b@0 0000 putchilledstring "x" ( 1)[Li] 0002 setlocal_WC_0 b@0 0004 putself 0005 opt_send_without_block <calldata!mid:v, argc:0, FCALL|VCALL|ARGS_SIMPLE> 0007 putchilledstring "E*" 0009 getlocal b@0, 0 0012 opt_newarray_send 3, 5 0015 leave ```
1 parent 86a762c commit acbb8d4

File tree

12 files changed

+205
-40
lines changed

12 files changed

+205
-40
lines changed

‎bootstraptest/test_insns.rb

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,48 @@ def min
236236
end
237237
[3, x = 2, 1].min
238238
},
239+
[ 'opt_newarray_send', %q{ v = 1.23; [v, v*2].pack("E*").unpack("E*") == [v, v*2] }, ],
240+
[ 'opt_newarray_send', %q{ v = 4.56; b = +"x"; [v, v*2].pack("E*", buffer: b); b[1..].unpack("E*") == [v, v*2] }, ],
241+
[ 'opt_newarray_send', <<-'},', ], # {
242+
v = 7.89;
243+
b = +"x";
244+
class Array
245+
alias _pack pack
246+
def pack(s, buffer: nil, prefix: "y")
247+
buffer ||= +"b"
248+
buffer << prefix
249+
_pack(s, buffer: buffer)
250+
end
251+
end
252+
tests = []
253+
254+
ret = [v].pack("E*", prefix: "z")
255+
tests << (ret[0..1] == "bz")
256+
tests << (ret[2..].unpack("E*") == [v])
257+
258+
ret = [v].pack("E*")
259+
tests << (ret[0..1] == "by")
260+
tests << (ret[2..].unpack("E*") == [v])
261+
262+
[v, v*2, v*3].pack("E*", buffer: b)
263+
tests << (b[0..1] == "xy")
264+
tests << (b[2..].unpack("E*") == [v, v*2, v*3])
265+
266+
class Array
267+
def pack(_fmt, buffer:) = buffer
268+
end
269+
270+
b = nil
271+
tests << [v].pack("E*", buffer: b).nil?
272+
273+
class Array
274+
def pack(_fmt, **kw) = kw.empty?
275+
end
276+
277+
tests << [v].pack("E*") == true
278+
279+
tests.all? or puts tests
280+
},
239281

240282
[ 'throw', %q{ false.tap { break true } }, ],
241283
[ 'branchif', %q{ x = nil; x ||= true }, ],

‎bootstraptest/test_yjit.rb

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5185,3 +5185,19 @@ def test
51855185
51865186
test
51875187
RUBY
5188+
5189+
assert_equal '[true, true]', <<~'RUBY'
5190+
def pack
5191+
v = 1.23
5192+
[v, v*2, v*3].pack("E*").unpack("E*") == [v, v*2, v*3]
5193+
end
5194+
5195+
def with_buffer
5196+
v = 4.56
5197+
b = +"x"
5198+
[v, v*2, v*3].pack("E*", buffer: b)
5199+
b[1..].unpack("E*") == [v, v*2, v*3]
5200+
end
5201+
5202+
[pack, with_buffer]
5203+
RUBY

‎compile.c

Lines changed: 47 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -3996,27 +3996,35 @@ iseq_specialized_instruction(rb_iseq_t *iseq, INSN *iobj)
39963996
if (IS_INSN_ID(iobj, newarray) && iobj->link.next &&
39973997
IS_INSN(iobj->link.next)) {
39983998
/*
3999-
* [a, b, ...].max/min -> a, b, c, opt_newarray_max/min
3999+
* [a, b, ...].max/min -> a, b, c, opt_newarray_send max/min
40004000
*/
40014001
INSN *niobj = (INSN *)iobj->link.next;
40024002
if (IS_INSN_ID(niobj, send)) {
40034003
const struct rb_callinfo *ci = (struct rb_callinfo *)OPERAND_AT(niobj, 0);
40044004
if (vm_ci_simple(ci) && vm_ci_argc(ci) == 0) {
4005+
VALUE method = INT2FIX(0);
40054006
switch (vm_ci_mid(ci)) {
40064007
case idMax:
4008+
method = INT2FIX(VM_OPT_NEWARRAY_SEND_MAX);
4009+
break;
40074010
case idMin:
4011+
method = INT2FIX(VM_OPT_NEWARRAY_SEND_MIN);
4012+
break;
40084013
case idHash:
4009-
{
4010-
VALUE num = iobj->operands[0];
4011-
int operand_len = insn_len(BIN(opt_newarray_send)) - 1;
4012-
iobj->insn_id = BIN(opt_newarray_send);
4013-
iobj->operands = compile_data_calloc2(iseq, operand_len, sizeof(VALUE));
4014-
iobj->operands[0] = num;
4015-
iobj->operands[1] = rb_id2sym(vm_ci_mid(ci));
4016-
iobj->operand_size = operand_len;
4017-
ELEM_REMOVE(&niobj->link);
4018-
return COMPILE_OK;
4019-
}
4014+
method = INT2FIX(VM_OPT_NEWARRAY_SEND_HASH);
4015+
break;
4016+
}
4017+
4018+
if (method != INT2FIX(0)) {
4019+
VALUE num = iobj->operands[0];
4020+
int operand_len = insn_len(BIN(opt_newarray_send)) - 1;
4021+
iobj->insn_id = BIN(opt_newarray_send);
4022+
iobj->operands = compile_data_calloc2(iseq, operand_len, sizeof(VALUE));
4023+
iobj->operands[0] = num;
4024+
iobj->operands[1] = method;
4025+
iobj->operand_size = operand_len;
4026+
ELEM_REMOVE(&niobj->link);
4027+
return COMPILE_OK;
40204028
}
40214029
}
40224030
}
@@ -4030,14 +4038,40 @@ iseq_specialized_instruction(rb_iseq_t *iseq, INSN *iobj)
40304038
iobj->insn_id = BIN(opt_newarray_send);
40314039
iobj->operands = compile_data_calloc2(iseq, operand_len, sizeof(VALUE));
40324040
iobj->operands[0] = FIXNUM_INC(num, 1);
4033-
iobj->operands[1] = rb_id2sym(vm_ci_mid(ci));
4041+
iobj->operands[1] = INT2FIX(VM_OPT_NEWARRAY_SEND_PACK);
40344042
iobj->operand_size = operand_len;
40354043
ELEM_REMOVE(&iobj->link);
40364044
ELEM_REMOVE(niobj->link.next);
40374045
ELEM_INSERT_NEXT(&niobj->link, &iobj->link);
40384046
return COMPILE_OK;
40394047
}
40404048
}
4049+
// newarray n, putchilledstring "E", getlocal b, send :pack with {buffer: b}
4050+
// -> putchilledstring "E", getlocal b, opt_newarray_send n+2, :pack, :buffer
4051+
else if ((IS_INSN_ID(niobj, putstring) || IS_INSN_ID(niobj, putchilledstring) ||
4052+
(IS_INSN_ID(niobj, putobject) && RB_TYPE_P(OPERAND_AT(niobj, 0), T_STRING))) &&
4053+
IS_NEXT_INSN_ID(&niobj->link, getlocal) &&
4054+
(niobj->link.next && IS_NEXT_INSN_ID(niobj->link.next, send))) {
4055+
const struct rb_callinfo *ci = (struct rb_callinfo *)OPERAND_AT((INSN *)(niobj->link.next)->next, 0);
4056+
const struct rb_callinfo_kwarg *kwarg = vm_ci_kwarg(ci);
4057+
if (vm_ci_mid(ci) == idPack && vm_ci_argc(ci) == 2 &&
4058+
(kwarg && kwarg->keyword_len == 1 && kwarg->keywords[0] == rb_id2sym(idBuffer))) {
4059+
VALUE num = iobj->operands[0];
4060+
int operand_len = insn_len(BIN(opt_newarray_send)) - 1;
4061+
iobj->insn_id = BIN(opt_newarray_send);
4062+
iobj->operands = compile_data_calloc2(iseq, operand_len, sizeof(VALUE));
4063+
iobj->operands[0] = FIXNUM_INC(num, 2);
4064+
iobj->operands[1] = INT2FIX(VM_OPT_NEWARRAY_SEND_PACK_BUFFER);
4065+
iobj->operand_size = operand_len;
4066+
// Remove the "send" insn.
4067+
ELEM_REMOVE((niobj->link.next)->next);
4068+
// Remove the modified insn from its original "newarray" position...
4069+
ELEM_REMOVE(&iobj->link);
4070+
// and insert it after the buffer insn.
4071+
ELEM_INSERT_NEXT(niobj->link.next, &iobj->link);
4072+
return COMPILE_OK;
4073+
}
4074+
}
40414075
}
40424076

40434077
if (IS_INSN_ID(iobj, send)) {

‎defs/id.def

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ firstline, predefined = __LINE__+1, %[\
6060
nil
6161
path
6262
pack
63+
buffer
6364
6465
_ UScore
6566

‎insns.def

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -983,7 +983,7 @@ opt_str_uminus
983983

984984
DEFINE_INSN
985985
opt_newarray_send
986-
(rb_num_t num, ID method)
986+
(rb_num_t num, rb_num_t method)
987987
(...)
988988
(VALUE val)
989989
/* This instruction typically has no funcalls. But it compares array
@@ -995,17 +995,20 @@ opt_newarray_send
995995
// attr rb_snum_t comptime_sp_inc = 1 - (rb_snum_t)num;
996996
{
997997
switch(method) {
998-
case idHash:
998+
case VM_OPT_NEWARRAY_SEND_HASH:
999999
val = vm_opt_newarray_hash(ec, num, STACK_ADDR_FROM_TOP(num));
10001000
break;
1001-
case idMin:
1001+
case VM_OPT_NEWARRAY_SEND_MIN:
10021002
val = vm_opt_newarray_min(ec, num, STACK_ADDR_FROM_TOP(num));
10031003
break;
1004-
case idMax:
1004+
case VM_OPT_NEWARRAY_SEND_MAX:
10051005
val = vm_opt_newarray_max(ec, num, STACK_ADDR_FROM_TOP(num));
10061006
break;
1007-
case idPack:
1008-
val = rb_vm_opt_newarray_pack(ec, (long)num-1, STACK_ADDR_FROM_TOP(num), TOPN(0));
1007+
case VM_OPT_NEWARRAY_SEND_PACK:
1008+
val = vm_opt_newarray_pack_buffer(ec, (long)num-1, STACK_ADDR_FROM_TOP(num), TOPN(0), Qundef);
1009+
break;
1010+
case VM_OPT_NEWARRAY_SEND_PACK_BUFFER:
1011+
val = vm_opt_newarray_pack_buffer(ec, (long)num-2, STACK_ADDR_FROM_TOP(num), TOPN(1), TOPN(0));
10091012
break;
10101013
default:
10111014
rb_bug("unreachable");

‎test/ruby/test_pack.rb

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -913,4 +913,27 @@ class Array
913913
assert_equal "oh no", v
914914
end;
915915
end
916+
917+
def test_monkey_pack_buffer
918+
assert_separately([], <<-'end;')
919+
$-w = false
920+
class Array
921+
alias :old_pack :pack
922+
def pack _, buffer:; buffer << " no"; end
923+
end
924+
925+
def test
926+
b = +"oh"
927+
[2 ** 15].pack('n', buffer: b)
928+
end
929+
930+
v = test
931+
932+
class Array
933+
alias :pack :old_pack
934+
end
935+
936+
assert_equal "oh no", v
937+
end;
938+
end
916939
end

‎vm_core.h

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1284,6 +1284,14 @@ enum vm_check_match_type {
12841284
#define VM_CHECKMATCH_TYPE_MASK 0x03
12851285
#define VM_CHECKMATCH_ARRAY 0x04
12861286

1287+
enum vm_opt_newarray_send_type {
1288+
VM_OPT_NEWARRAY_SEND_MAX = 1,
1289+
VM_OPT_NEWARRAY_SEND_MIN = 2,
1290+
VM_OPT_NEWARRAY_SEND_HASH = 3,
1291+
VM_OPT_NEWARRAY_SEND_PACK = 4,
1292+
VM_OPT_NEWARRAY_SEND_PACK_BUFFER = 5,
1293+
};
1294+
12871295
enum vm_special_object_type {
12881296
VM_SPECIAL_OBJECT_VMCORE = 1,
12891297
VM_SPECIAL_OBJECT_CBASE,

‎vm_insnhelper.c

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6203,19 +6203,45 @@ rb_vm_opt_newarray_hash(rb_execution_context_t *ec, rb_num_t num, const VALUE *p
62036203
VALUE rb_setup_fake_ary(struct RArray *fake_ary, const VALUE *list, long len, bool freeze);
62046204
VALUE rb_ec_pack_ary(rb_execution_context_t *ec, VALUE ary, VALUE fmt, VALUE buffer);
62056205

6206-
VALUE
6207-
rb_vm_opt_newarray_pack(rb_execution_context_t *ec, rb_num_t num, const VALUE *ptr, VALUE fmt)
6206+
static VALUE
6207+
vm_opt_newarray_pack_buffer(rb_execution_context_t *ec, rb_num_t num, const VALUE *ptr, VALUE fmt, VALUE buffer)
62086208
{
62096209
if (BASIC_OP_UNREDEFINED_P(BOP_PACK, ARRAY_REDEFINED_OP_FLAG)) {
62106210
struct RArray fake_ary;
62116211
VALUE ary = rb_setup_fake_ary(&fake_ary, ptr, num, true);
6212-
return rb_ec_pack_ary(ec, ary, fmt, Qnil);
6212+
return rb_ec_pack_ary(ec, ary, fmt, (UNDEF_P(buffer) ? Qnil : buffer));
62136213
}
62146214
else {
6215-
return rb_vm_call_with_refinements(ec, rb_ary_new4(num, ptr), idPack, 1, &fmt, RB_PASS_CALLED_KEYWORDS);
6215+
// The opt_newarray_send insn drops the keyword args so we need to rebuild them.
6216+
// Setup an array with room for keyword hash.
6217+
VALUE args[2];
6218+
args[0] = fmt;
6219+
int kw_splat = RB_NO_KEYWORDS;
6220+
int argc = 1;
6221+
6222+
if (!UNDEF_P(buffer)) {
6223+
args[1] = rb_hash_new_with_size(1);
6224+
rb_hash_aset(args[1], ID2SYM(idBuffer), buffer);
6225+
kw_splat = RB_PASS_KEYWORDS;
6226+
argc++;
6227+
}
6228+
6229+
return rb_vm_call_with_refinements(ec, rb_ary_new4(num, ptr), idPack, argc, args, kw_splat);
62166230
}
62176231
}
62186232

6233+
VALUE
6234+
rb_vm_opt_newarray_pack_buffer(rb_execution_context_t *ec, rb_num_t num, const VALUE *ptr, VALUE fmt, VALUE buffer)
6235+
{
6236+
return vm_opt_newarray_pack_buffer(ec, num, ptr, fmt, buffer);
6237+
}
6238+
6239+
VALUE
6240+
rb_vm_opt_newarray_pack(rb_execution_context_t *ec, rb_num_t num, const VALUE *ptr, VALUE fmt)
6241+
{
6242+
return vm_opt_newarray_pack_buffer(ec, num, ptr, fmt, Qundef);
6243+
}
6244+
62196245
#undef id_cmp
62206246

62216247
#define IMEMO_CONST_CACHE_SHAREABLE IMEMO_FL_USER0

‎yjit/bindgen/src/main.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -300,6 +300,7 @@ fn main() {
300300
.allowlist_type("ruby_tag_type")
301301
.allowlist_type("ruby_vm_throw_flags")
302302
.allowlist_type("vm_check_match_type")
303+
.allowlist_type("vm_opt_newarray_send_type")
303304
.allowlist_type("rb_iseq_type")
304305

305306
// From yjit.c

‎yjit/src/codegen.rs

Lines changed: 22 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -4157,47 +4157,56 @@ fn gen_opt_newarray_send(
41574157
jit: &mut JITState,
41584158
asm: &mut Assembler,
41594159
) -> Option<CodegenStatus> {
4160-
let method = jit.get_arg(1).as_u64();
4160+
let method = jit.get_arg(1).as_u32();
41614161

4162-
if method == ID!(min) {
4162+
if method == VM_OPT_NEWARRAY_SEND_MIN {
41634163
gen_opt_newarray_min(jit, asm)
4164-
} else if method == ID!(max) {
4164+
} else if method == VM_OPT_NEWARRAY_SEND_MAX {
41654165
gen_opt_newarray_max(jit, asm)
4166-
} else if method == ID!(hash) {
4166+
} else if method == VM_OPT_NEWARRAY_SEND_HASH {
41674167
gen_opt_newarray_hash(jit, asm)
4168-
} else if method == ID!(pack) {
4169-
gen_opt_newarray_pack(jit, asm)
4168+
} else if method == VM_OPT_NEWARRAY_SEND_PACK {
4169+
gen_opt_newarray_pack_buffer(jit, asm, 1, None)
4170+
} else if method == VM_OPT_NEWARRAY_SEND_PACK_BUFFER {
4171+
gen_opt_newarray_pack_buffer(jit, asm, 2, Some(1))
41704172
} else {
41714173
None
41724174
}
41734175
}
41744176

4175-
fn gen_opt_newarray_pack(
4177+
fn gen_opt_newarray_pack_buffer(
41764178
jit: &mut JITState,
41774179
asm: &mut Assembler,
4180+
fmt_offset: u32,
4181+
buffer: Option<u32>,
41784182
) -> Option<CodegenStatus> {
4179-
// num == 4 ( for this code )
4183+
asm_comment!(asm, "opt_newarray_send pack");
4184+
41804185
let num = jit.get_arg(0).as_u32();
41814186

41824187
// Save the PC and SP because we may call #pack
41834188
jit_prepare_non_leaf_call(jit, asm);
41844189

41854190
extern "C" {
4186-
fn rb_vm_opt_newarray_pack(ec: EcPtr, num: u32, elts: *const VALUE, fmt: VALUE) -> VALUE;
4191+
fn rb_vm_opt_newarray_pack_buffer(ec: EcPtr, num: u32, elts: *const VALUE, fmt: VALUE, buffer: VALUE) -> VALUE;
41874192
}
41884193

41894194
let values_opnd = asm.ctx.sp_opnd(-(num as i32));
41904195
let values_ptr = asm.lea(values_opnd);
41914196

4192-
let fmt_string = asm.ctx.sp_opnd(-1);
4197+
let fmt_string = asm.ctx.sp_opnd(-(fmt_offset as i32));
41934198

41944199
let val_opnd = asm.ccall(
4195-
rb_vm_opt_newarray_pack as *const u8,
4200+
rb_vm_opt_newarray_pack_buffer as *const u8,
41964201
vec![
41974202
EC,
4198-
(num - 1).into(),
4203+
(num - fmt_offset).into(),
41994204
values_ptr,
4200-
fmt_string
4205+
fmt_string,
4206+
match buffer {
4207+
None => Qundef.into(),
4208+
Some(i) => asm.ctx.sp_opnd(-(i as i32)),
4209+
},
42014210
],
42024211
);
42034212

0 commit comments

Comments
 (0)