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

[Vladimir Kozlov]

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.

I did say 1224: "I suggest to change #ifndef PRODUCT at line 1224 to #ifdef ASSERT"

As Christian said, use `NOT_DEBUG_RETURN` for cases when the method body is defined only in debug build (under `#ifdef ASSERT`).

Note, my suggestion for changing `!PRODUCT` to `ASSERT` at line 1224 and build `optimized` VM was experiment we frequently use to find which functions are not use in `optimized` VM and which are used. It is usually faster than tracing usage in sources. I don't mean to use it in final changes. By using this experiment you can separate methods between `!PRODUCT` and `ASSERT`.

Please,  use `#ifdef ASSERT` or `DEBUG_ONLY()` instead of `__DO_DELAYED_VERIFICATION`. 

> 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.

Yes, this is correct. The assertion/verification is used in our regular testing when we use fastdebug builds to catch issues.
Optimized build is mostly used to understand what is going on in build which is very similar to product.

Yes, we have RFE to remove optimized build and replace it with separate value check:
https://bugs.openjdk.java.net/browse/JDK-8202283

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

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