RFR: JDK-8258603 c1 IR::verify is expensive [v5]

[Christian Hagedorn]

On Thu, 23 Dec 2021 01:25:39 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>>> > So the whole block of code in `c1_IR.cpp` is under `#ifndef PRODUCT`. But is it used in **optimized** build or only in debug?
>>> > I suggest to change `#ifndef PRODUCT` at line 1224 to `#ifdef ASSERT` and try to build optimized VM to see if it is used in it. I see only prints and asserts.
>>> 
>>> Shouldn't we leave `BlockPrinter` and `IR::print()` under `#ifndef PRODUCT`? These seem to be about printing things only which is probably good to have in optimized builds as well. We could instead just add an `#ifdef ASSERT` on L1263 for the validators and the IR verification and change the places using this code accordingly to `#ifdef ASSERT/DEBUG_ONLY()`. Then we would have everything under `ASSERT` instead of `!PRODUCT`. This would be in line with the original code which used `#ifdef ASSERT` in `IR::verify()`.
>> 
>> I agree with christian's suggestion.
>
>> @vnkozlov I don't know what an optimized build is, you'll have to point me to some instructions if you want me to build one.
> 
> 
> bash configure --with-conf-name=optimized --with-debug-level=optimized
> make jdk-image CONF=optimized
> ./build/optimized/images/jdk/bin/java -XX:+PrintCFG -version

> @vnkozlov You proposed changing c1_IR.cpp:1244 (I think you meant :1224) to `#ifdef ASSERT`. I tried it with an optimized build and this gave linking errors, specifically "undefined reference to IR::verify()." Seems likely to me that this is caused by mismatch withuse of `PRODUCT_RETURN` in c1_IR.hpp.

When applying these changes:

> > > So the whole block of code in `c1_IR.cpp` is under `#ifndef PRODUCT`. But is it used in **optimized** build or only in debug?
> > > I suggest to change `#ifndef PRODUCT` at line 1224 to `#ifdef ASSERT` and try to build optimized VM to see if it is used in it. I see only prints and asserts.
> > 
> > 
> > Shouldn't we leave `BlockPrinter` and `IR::print()` under `#ifndef PRODUCT`? These seem to be about printing things only which is probably good to have in optimized builds as well. We could instead just add an `#ifdef ASSERT` on L1263 for the validators and the IR verification and change the places using this code accordingly to `#ifdef ASSERT/DEBUG_ONLY()`. Then we would have everything under `ASSERT` instead of `!PRODUCT`. This would be in line with the original code which used `#ifdef ASSERT` in `IR::verify()`.
> 
> I agree with christian's suggestion.

Then you can change `PRODUCT_RETURN` into `NOT_DEBUG_RETURN` to compile it and change `__DO_DELAYED_VERIFICATION` back to `ASSERT`.

My understanding is that assertion/verification code should be under `ASSERT` while additional debugging/printing code that is not available in product should be under `!PRODUCT`. But I don't know if that is documented somewhere officially and there are some places where it is mixed. I think there were also some discussions to move away from optimized builds completely.

-------------

PR: https://git.openjdk.java.net/jdk/pull/6850