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

[Igor Veresov]

Tom, thanks a lot for the review!
Please find responses inlined.

Updated webrev: http://cr.openjdk.java.net/~iveresov/6985015/webrev.01/

igor

On 11/19/10 1:18 PM, Tom Rodriguez wrote:
> The cpu_reg_range_reduction stuff is pretty gross.  Can we just make nof_caller_save_cpu_regs a method on FrameMap.  The existing enum would become _max_nof_caller_save_cpu_regs so it could continue to be a constant for declarations.
>

Sure, it seemed to be a good idea a the time, I thought it would look 
more uniform. Anyways, done, and added 2 more of these functions: for 
last_cpu_reg and last_byte_reg.

> assembler_sparc.cpp:
>
> what is slr and why would you use it instead of sllx?

Because sllx won't clear the upper bits. I thought I'd piggyback this 
little fix on this change.

>
> c1_LIRAssembler_sparc.cpp:
>
> use load_klass instead.

It this about vtable_call() ? There was an issue with the SS12u1, I had 
to either hand-inline it, or lower the opt level. I'll put a comment.

>
> In general if you can use load_heap_oop and store_heap_oop instead that would be preferable.  It's possible you'll need to add an alternate temp argument to them since they always assume it's ok to kill the oop value.
>

But to do that I would probably have to pass "wide" there and also to 
return the offset of the instruction that contains disp for patching. I 
thought I'd just contain this oddity in load/store or mem2reg/reg2mem.

I can introduce these primitives if you like though...

> in const2mem, init offset to -1 and assert that it's been set at before using it.
>

Done.

> sometimes the argument ordering is wide then unaligned and other times it's reversed which seems like asking for trouble.  Why is it like this?

Fixed.

>
> I guess the arraycopy guards were fairly broken on 64 bit since they were still using br instead of brx in a few places.  Oops.
>
> You're loading the klasses as 32 bit but comparing them using the 64 bit condition code which I know will work in this case but it's a bad practice.
 > loads always produce safe values but it's not clear that our constant 
construction code does.  Maybe we need a br variant that allows 
specifying the
 > condition codes? bp lets you but that's v9 only.
Done.
>
> + #ifdef _LP64
> +     if (UseCompressedOops) {
> +       __ encode_heap_oop(tmp, tmp);
> +       __ lduw(dst, oopDesc::klass_offset_in_bytes(), tmp2);
> +     } else
> + #endif
>          __ ld_ptr(dst, oopDesc::klass_offset_in_bytes(), tmp2);
>
> Could you put some comments on this?

Sure.

> I know this code is assert only but is there a need for a jobject2reg that produces the encoded oop directly?  The relocs support it.
> Doing the type checks by producing wide oops will be much more expensive.

Yes, and there is definitely an opportunity to optimize checkcast and 
instanceof by doing that. I'd like address that in a separate CR, if 
that's ok?

>
> c1_LIRAssembler_x86.cpp:
>
> what happening with the alignment?  Why isn't it aligned from compressed oops?

On x86 all compressed oops operations (like *code_heap_oop() or 
load/store_klass()) contain heapbase verfication, so the size of the 
emitted code changes not only for 32 to 64 bits but also depending on 
whether the ASSERT is on. So, I decided I'd just stick with the 
post-padding.

>
> Other it looks fine.
>
> tom
>
> On Nov 18, 2010, at 7:12 PM, 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/
>>
>> Thanks,
>> igor
>