[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-compiler-dev
Subject:    Re: Preliminary RFR (L): 8001107: @Stable annotation for constant folding of lazily evaluated variab
From:       Vladimir Ivanov <vladimir.x.ivanov () oracle ! com>
Date:       2013-07-26 21:53:53
Message-ID: 51F2EFF1.3020501 () oracle ! com
[Download RAW message or body]

Updated according to comments from Chris:
http://cr.openjdk.java.net/~vlivanov/8001107/webrev.02

Best regards,
Vladimir Ivanov

On 7/23/13 9:31 PM, Vladimir Ivanov wrote:
> Rickard,
>
> Thank you for review.
> See my answers inline.
>
> Updated version:
> http://cr.openjdk.java.net/~vlivanov/8001107/webrev.01
>
> Best regards,
> Vladimir Ivanov
>
> On 7/23/13 2:46 PM, Rickard Bäckman wrote:
>> Vladimir,
>>
>> first, nice work.
>>
>> A couple of comments about the code:
>>
>> ciArray.hpp / ciArray.cpp
>> Looks like most of the new methods could be declared const.
> ciObject::klass() isn't const, so newly introduced methods can't be made
> const as well.
>
>> ciConstant.hpp
>> I would like to have braces add to the if / else if / else
> Done.
>
>> classFileParser.hpp
>> The is_stable() should be declared const.
> Done.
>
>> memnode.cpp
>> You are adding plenty of logic to an already very long method, could
>> we extract the new parts to its own method? or methods.
> Done. Introduced fold_stable_ary_elem helper function.
>
>> The elembt local variable is never really used.
> Removed.
>
>> type.cpp
>> 2 instances of assert(stable == true || stable == false, ""); - this
>> seems to be always true?
> Agree. Removed.
>
>> The constant 42 in the hash, since we are hashing pointers it might be
>> more effective to use and odd number?
> Done.
>
>> 3505: if (stable_dimension <= 0 || stable_dimension == 1 && stable ==
>> this->stable())
>> should have parantheses added to clarify the || && logic.
> Done.
>
>> 3511   if (stable_dimension > 1 && elem_ptr != NULL &&
>> elem_ptr->base() == Type::AryPtr) {
>> could use the is_aryptr() instead of comparing base to Type::AryPtr
> Done.
>
>> type.hpp
>> the getter is named stable(), in all other places it is is_stable()
>> maybe we could rename stable -> is_stable.
> Done.
>
>> Cheers
>> /R
>>
>> On Jul 20, 2013, at 12:19 AM, Vladimir Ivanov wrote:
>>
>>> http://cr.openjdk.java.net/~vlivanov/8001107/
>>> 1455 lines changed: 1366 ins; 34 del; 55 mod;
>>>
>>> 8001107: @Stable annotation for constant folding of lazily evaluated
>>> variables
>>>
>>> Excerpt from Javadoc:
>>> [@Stable annotation is] Internal marker for some fields in the JSR
>>> 292 implementation. A field may be annotated as stable if all of its
>>> component variables changes value at most once. A field's value
>>> counts as its component value. If the field is typed as an array,
>>> then all the non-null components of the array, of depth up to the
>>> rank of the field's array type, also count as component values. By
>>> extension, any variable (either array or field) which has annotated
>>> as stable is called a stable variable, and its non-null or non-zero
>>> value is called a stable value.
>>>
>>> Testing: unit test, JPRT, VM testbase, nashorn, octane
>>>
>>> Contributed-by: jrose, vlivanov
>>>
>>> I marked review request as preliminary, because I think it may be too
>>> late to get it into 8. It'll be targeted for 8update then.
>>>
>>> Thanks!
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>
[prev in list] [next in list] [prev in thread] [next in thread]