Request for reviews (S): 8002069: Assert failed in C2: assert(field->edge_count() > 0) failed: sanity

[Vladimir Kozlov]

Vladimir Ivanov wrote:
> Ah, I see. Missed that difference between develop vs notproduct flags.
> 
> Are there any guidelines when a flag should be "notproduct" and when 
> "develop"?

Originally there were no notproduct flags. But we had a lot of cases like this:

#ifdef ASSERT
if (Flag) {
   n->dump(); // dump() is only defined in debug build
              // to reduce C++ vtable size (we have several
              // hundreds ideal+mach nodes)
   assert(check,"");
}
#endif

Where Flag was used only in such cases in debug VM. So "notproduct" type was 
added to reduce size of product VM (7 years ago):

6258968: Global flags cleanup for C1 and C2

Vladimir K.

> 
> Best regards,
> Vladimir Ivanov
> 
> On 11/7/12 1:38 AM, Vladimir Kozlov wrote:
>> Thank you, Vladimir
>>
>> There are 3 major kinds of flags:
>>
>> product - available in all versions of VM builds (debug, product,
>> optimized)
>> develop - also available in all versions of VM build, but declared as
>>            a constant value in non-debug builds (product, optimized) -
>>            you can change its value on command line only in debug build
>> notproduct - declared only in non-product builds (debug,
>>               optimized), have to be enclosed by #ifdef ASSERT or
>>               #ifndef PRODUCT.
>>
>> Regards,
>> Vladimir K.
>>
>> Vladimir Ivanov wrote:
>>> Vladimir K.,
>>>
>>> I'd like to clarify one thing with code style.
>>>
>>> Why do you guard verification code with ASSERT?
>>> Isn't it enough to have VerifyConnectionGraph non-product [1], so
>>> compiler can just eliminate relevant code as dead?
>>>
>>> As a use case: it's much easier and less error-prone to enable
>>> non-product code guarded by a flag just by changing flag type rather
>>> than replacing all relevant asserts. I envision such situations be
>>> useful during development/testing/problem diagnostics.
>>>
>>> Otherwise, the fix looks good.
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>>
>>> [1] src/share/vm/opto/c2_globals.hpp:460:  notproduct(bool,
>>> VerifyConnectionGraph , true,
>>>
>>> On 11/3/12 2:34 AM, Vladimir Kozlov wrote:
>>>> http://cr.openjdk.java.net/~kvn/8002069/webrev/
>>>>
>>>> The reference to a field on a dead path (which is not eliminated 
>>>> yet) of
>>>> bimorphic inlined call has different type (oop) instead of real (int)
>>>> field type.
>>>>
>>>> Added missed type check:
>>>>
>>>> -          if (store != NULL && store->is_Store()) {
>>>> +          if (store != NULL && store->is_Store() &&
>>>> +              store->as_Store()->memory_type() == ft) {
>>>>
>>>> The rest of changes are additional verification and verbose output
>>>> during failure which helped me to debug the problem.
>>>>
>>>> Added compiler regression test.
>>>>
>>>> Tested with CTW and compiler regression tests.
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>