RFR (L): 7153771: array bound check elimination for c1

[Vladimir Kozlov]

My main complain about code in c1_RangeCheckElimination.cpp is 
assignments inside condition checks:

if (c = lo->x()->as_Constant()) {

if ((op2 = v->as_Op2())) {

and others. Is it normal coding style in C1?

"integer" is bad argument name: , int integer,

Did you run JPRT and compiler regression tests?

Vladimir

On 2/21/13 1:03 PM, Vladimir Kozlov wrote:
> I am not familiar with C1 so my questions mostly general.
>
> Did C1 compilation time increased? And how much?
>
> We usually generate assert checks for debug builds only to get
> 'optimized' build without asserts. Also AssertRangeCheckElimination is
> develop flag. So could you used #ifdef ASSERT instead of #ifndef PRODUCT
> for Assert operatioins?
>
> Why in RangeCheckStub::emit_code() there is no ce->verify_oop_map(_info)
> in new code (in both sparc and x86 files )?
> Also ImplicitNullCheckStub::emit_code() on sparc has verify_oop_map()
> call but x86 does not.
>
> c1_LIRAssembler_sparc.cpp spacing: remove one empty line before new code
> and add new line after the code.
>
> c1_Runtime1_sparc.cpp why set_info() message is "incompatible_class_cast"?
>
> c1_Compilation.cpp the orders in TimerName and timer_name[] are not
> matching.
>
> Why you removed ResourceMark? Is it because resource area is not used?
> Or some resource allocated objects are used after that code? How it is
> related to ValueStack objects?:
>
>     if (UseGlobalValueNumbering) {
> -    ResourceMark rm;
> +    // No resource mark here! LoopInvariantCodeMotion can allocate
> ValueStack objects.
>
>
> Why not use Deoptimization::Reason_range_check? You use Reason_none.
>
> c1_GraphBuilder.cpp has repeated pattern (could be factored out):
>
>    (is_bb || compilation()->is_optimistic()) ? copy_state_before() : NULL
>
> Code style: open "{" should on for() line:
>
> +      for (int j = 0; j < loop_header->number_of_preds(); j++)
> +      {
>
> c1_Instruction.hpp how you get float/double compares for range check?:
>
> +  // unordered_is_true is valid for float/double compares only
>
> c1_RangeCheckElimination.cpp, why you do RCE when
> AssertRangeCheckElimination is set but there are no arrays references in
> a loop? Is it typo and you wanted it for StressRangeCheckElimination
> instead? :
>
>    46 void RangeCheckElimination::eliminate(IR *ir) {
>    47   bool do_elimination = ir->compilation()->has_access_indexed();
>    48   ASSERT_RANGE_CHECK_ELIMINATION(do_elimination = true);
>    49   if (do_elimination) {
>    50     RangeCheckEliminator rce(ir);
>    51   }
>    52 }
>    53
>
> I will look more on code in c1_RangeCheckElimination.cpp later.
>
> Thanks,
> Vladimir
>
> On 2/20/13 8:51 AM, Roland Westrelin wrote:
>> Here is a new version of the webrev that addresses Tom's comments.
>>
>> http://cr.openjdk.java.net/~roland/7153771/webrev.01/
>>
>>> Should this reuse C2 flags like RangeCheckElimination instead of
>>> introducing it's own?
>>
>> I only reused RangeCheckElimination. I wasn't sure which others it
>> made sense to re-use.
>>
>> Roland.
>>