Split cmp operations that aren't 32/64 bit for arm #6484
Conversation
|
See #6475 for previous approach |
| match opnd0 { | ||
| Opnd::Reg(_) | Opnd::InsnOut { .. } => { | ||
| match opnd0.rm_num_bits() { | ||
| 8 => asm.and(opnd0.with_num_bits(64).unwrap(), Opnd::UImm(0xff)), |
There was a problem hiding this comment.
I think both of these should be with_num_bits(32) since we can use the smaller comparison. I don't know if it will necessarily be that much faster, but it could help.
There was a problem hiding this comment.
Thanks for reviewing this @kddnewton
I think Kevin is right, but I also think you might need to split and potentially load opnd0 here, because and can't accept memory operands.
There was a problem hiding this comment.
I would love if that were true. We have a mistake in our assembler for 32bit and on arm. It generates invalid machine code. I plan on fixing it. But didn't want to expand the scope more.
There was a problem hiding this comment.
🤦♀️ okay one fix at a time 😅
There was a problem hiding this comment.
Right before this is called there is (see in the diff below)
let opnd0 = split_load_operand(asm, left);
So it felt a bit weird doing it again. I can if that is the preference.
There was a problem hiding this comment.
I hadn't spotted that. Looks good Jimmy, well-done 👍
This is very targeted just at the use case in guard_known_klass for symbol. There are tons of ways make arm registers with
num_bits=8that will break. Maybe that is okay? Maybe it isn't? But as I looked at it, it seemed to unclear the right place/way to deal with that issue right now. Instead, I just targeted comparisons. I added a test that crashes on arm before this change. I also tried out guard_known_class for symbols on my optimize_send branch and everything worked as intended.