Request for review(XL): 6985015: C1 needs to support compressed oops

[Igor Veresov]

Christian, thanks for the review! Please find the responses inline.

On 11/19/10 2:51 AM, Christian Thalinger wrote:
> On Nov 19, 2010, at 4:12 AM, Igor Veresov wrote:
>> This change implements compressed oops for C1 for x64 and sparc.
>> The changes are mostly on the codegen level, with a few exceptions
>> when we do access things outside of the heap that are uncompressed
>> from the IR.
>>
>> Webrev: http://cr.openjdk.java.net/~iveresov/6985015/webrev.00/
>
> src/cpu/sparc/vm/c1_LIRAssembler_sparc.cpp:
>
> 764 #ifdef _LP64
> 765 if (UseCompressedOops) {
> 766 __ lduw(O0, oopDesc::klass_offset_in_bytes(), G3_scratch);
> 767 __ decode_heap_oop(G3_scratch);
> 768 } else
> 769 #endif
> 770 __ ld_ptr(O0, oopDesc::klass_offset_in_bytes(), G3_scratch);
> Why not using load_heap_oop here? I wonder why there is no
> load_klass_null that does all that (or actually the current load_klass
> should be called load_klass_not_null, but that's a too big change).

Yeah, I know. SS12u1 had problems in this place and it was either I 
hand-inline load_klass() or lower the optimization level. I'll put a 
comment there.

>
> 949 __ decode_heap_oop(to_reg->as_register(), to_reg->as_register());
> 993 __ decode_heap_oop(to_reg->as_register(), to_reg->as_register());
> Pass the register only once, it's a little shorter.
>

Yes, thanks.

> Maybe you should add a local method doing:
>
> 946 #ifdef _LP64
> 947 if (UseCompressedOops && !wide) {
> 948 __ lduw(base, offset, to_reg->as_register());
> 949 __ decode_heap_oop(to_reg->as_register(), to_reg->as_register());
> 950 } else
> 951 #endif
> 952 __ ld_ptr(base, offset, to_reg->as_register());
> and reuse it.
>

I will be two different methods anyway, since one will have to accept 
the offset as a register, and the other as an int. If you think such a 
factoring will increase readability, then, sure I'll make these 
constructs as functions.

> 1240 // load(O7, 0, to_reg, T_INT, false /*wide*/, false /*unaligned*/);
> Why is it commented?
>

Forgot to remove it, thanks!

> src/cpu/x86/vm/c1_LIRAssembler_x86.cpp:
>
> 550 __ load_heap_oop(rsi, Address(rax,
> java_lang_String::value_offset_in_bytes()));
> 551 __ movptr (rcx, Address(rax,
> java_lang_String::offset_offset_in_bytes()));
> 552 __ lea (rsi, Address(rsi, rcx, Address::times_2,
> arrayOopDesc::base_offset_in_bytes(T_CHAR)));
> 557 __ load_heap_oop(rdi, Address(rbx,
> java_lang_String::value_offset_in_bytes()));
> 558 __ movptr (rcx, Address(rbx,
> java_lang_String::offset_offset_in_bytes()));
> 559 __ lea (rdi, Address(rdi, rcx, Address::times_2,
> arrayOopDesc::base_offset_in_bytes(T_CHAR)));
> Please keep the indent.
>

Done.

> 779 case T_ADDRESS:
> 780 __ movptr(frame_map()->address_for_slot(dest->single_stack_ix()),
> c->as_jint_bits());
> 781 break;
> Indent.
>

Done.

> src/cpu/x86/vm/c1_LIRGenerator_x86.cpp:
>
> 1143 if (!x->klass()->is_loaded() || LP64_ONLY(UseCompressedOops)
> NOT_LP64(false)) {
> 1165 if (!x->klass()->is_loaded() || LP64_ONLY(UseCompressedOops)
> NOT_LP64(false)) {
> I think the common pattern is LP64_ONLY(|| UseCompressedOops).

It is, but do you think it would be more readable to factor this 
predicate to separate function. It just determines if we need an extra 
temporary. I'd rather leave it as is.

>
> src/share/vm/c1/c1_LIRAssembler.hpp:
>
> void mem2reg (LIR_Opr src, LIR_Opr dest, BasicType type,
> - LIR_PatchCode patch_code = lir_patch_none,
> - CodeEmitInfo* info = NULL, bool unaligned = false);
> + LIR_PatchCode patch_code,
> + CodeEmitInfo* info, bool unaligned, bool wide);
> Why did you remove the default values? Because wide is always passed in?

It is used only in a very few places, so I don't think saving on extra 
typing is that beneficial. The arguments where also specified every 
time, so their defaultness was never exploited. In general I prefer to 
avoid default arguments as much as possible.


>
> Otherwise looks good.
>
> -- Christian

Thanks!
igor