RFR: 8254129: IR Test Framework to support regex-based matching on the IR in JTreg compiler tests [v3]

Tue Apr 20 13:42:17 UTC 2021

On Mon, 19 Apr 2021 12:49:39 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> This RFE provides an IR test framework to perform regex-based checks on the C2 IR shape of test methods emitted by the VM flags `-XX:+PrintIdeal` and `-XX:+PrintOptoAssembly`. The framework can also be used for other non-IR matching (and non-compiler) tests by providing easy to use annotations for commonly used testing patterns and compiler control flags.
>> 
>> The framework is based on the ideas of the currently present IR test framework in [Valhalla](https://github.com/openjdk/valhalla/blob/e9c78ce4fcfd01361c35883e0d68f9ae5a80d079/test/hotspot/jtreg/compiler/valhalla/inlinetypes/InlineTypeTest.java) (mainly implemented by @TobiHartmann) which is being used with great success. This new framework aims to replace the old one in Valhalla at some point.
>> 
>> A detailed description about how this new IR test framework works and how it is used is provided in the [README.md](https://github.com/chhagedorn/jdk/blob/aa005f384a4567c6c0b5f08f7c5df57f705dc540/test/lib/jdk/test/lib/hotspot/ir_framework/README.md) file and in the [Javadocs](https://github.com/chhagedorn/jdk/blob/aa005f384a4567c6c0b5f08f7c5df57f705dc540/test/lib/jdk/test/lib/hotspot/ir_framework/doc/jdk/test/lib/hotspot/ir_framework/package-summary.html) written for the framework classes.
>> 
>> To finish a first version of this framework for JDK 17, I decided to leave some improvement possibilities and ideas to be followed up on in additional RFEs. Some ideas are mentioned in "Future Work" in [README.md](https://github.com/chhagedorn/jdk/blob/aa005f384a4567c6c0b5f08f7c5df57f705dc540/test/lib/jdk/test/lib/hotspot/ir_framework/README.md) and were also created as subtasks of this RFE.
>> 
>> Testing (also described in "Internal Framework Tests in [README.md](https://github.com/chhagedorn/jdk/blob/aa005f384a4567c6c0b5f08f7c5df57f705dc540/test/lib/jdk/test/lib/hotspot/ir_framework/README.md)):
>> There are various tests to verify the correctness of the test framework which can be found as JTreg tests in the [tests](https://github.com/chhagedorn/jdk/tree/aa005f384a4567c6c0b5f08f7c5df57f705dc540/test/lib/jdk/test/lib/hotspot/ir_framework/tests) folder. Additional testing was performed by converting all compiler Inline Types test of project Valhalla (done by @katyapav in [JDK-8263024](https://bugs.openjdk.java.net/browse/JDK-8263024)) that used the old framework to the new framework. This provided additional testing for the framework itself. We ran the converted tests with all the flag settings used in hs-tier1-9 and hs-precheckin-comp. For sanity checking, this was also done with a sample IR test in mainline.
>> 
>> Some stats about the framework code added to [ir_framework](https://github.com/chhagedorn/jdk/tree/aa005f384a4567c6c0b5f08f7c5df57f705dc540/test/lib/jdk/test/lib/hotspot/ir_framework):
>> 
>> -  without the [Javadocs files](https://github.com/chhagedorn/jdk/tree/aa005f384a4567c6c0b5f08f7c5df57f705dc540/test/lib/jdk/test/lib/hotspot/ir_framework/doc) : 60 changed files, 13212 insertions, 0 deletions.
>>    - without the [tests](https://github.com/chhagedorn/jdk/tree/aa005f384a4567c6c0b5f08f7c5df57f705dc540/test/lib/jdk/test/lib/hotspot/ir_framework/tests)  and [examples](https://github.com/chhagedorn/jdk/tree/aa005f384a4567c6c0b5f08f7c5df57f705dc540/test/lib/jdk/test/lib/hotspot/ir_framework/examples) folder: 40 files changed, 6781 insertions
>>       - comments: 2399 insertions (calculated with `git diff --cached !(tests|examples) | grep -c -E "(^[+-]\s*(/)?*)|(^[+-]\s*//)"`)
>>       - which leaves 4382 lines of code inserted 
>> 
>> Big thanks to:
>> - @TobiHartmann for all his help by discussing the new framework and for providing insights from his IR test framework in Valhalla.
>> - @katyapav for converting the Valhalla tests to use the new framework which found some harder to catch bugs in the framework and also some actual C2 bugs.
>> - @iignatev for helping to simplify the framework usage with JTreg and with the framework internal VM calling structure.
>> - and others who provided valuable feedback.
>> 
>> Thanks,
>> Christian
>
> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Apply review comments in code from Igor I. and Vladimir K.

> > > * although having javadoc for testlibraries is highly desirable, I don't think we should check in the generated HTML files
> > > * the same goes for `README.html` generated from `README.md`
> > 
> > 
> > Okay, I will remove them. Does it make sense to still have the HTML files somewhere in the web, for example, on my cr.openjdk?
> > > * this library is hotspot-centric, I highly doubt that it will be used by any tests outside of the hotspot test base, hence the more appropriate location for it would be inside `test/hotspot/jtreg/testlibrary`.
> > > * I assume `test/lib/jdk/test/lib/hotspot/ir_framework/tests/` are the tests for the framework, in that case they should be in `test/lib-test`, if we stick to `test/lib` as the location for the library, or in `test/hotspot/jtreg/testlibrary_tests`, if we move it to `test/hotspot`
> > 
> > 
> > That makes sense to move everything to `test/hotspot/jtreg/testlibrary`. Right, the `test/lib/jdk/test/lib/hotspot/ir_framework/tests/` are only tests for the framework itself and should not be run as part of tier testing each time (does not make much sense) but only when the framework is actually modified. Is this still the case when putting them in `test/hotspot/jtreg/testlibrary_tests` (i.e. not executed unless run manually)?
> 
> `test/hotspot/jtreg/testlibrary_tests` are seldomly run as part of `:hotspot_misc` test group, yet I don't think it's an issue. the benefit of running testlibrary tests is to gain confidence that the tests which use these libraries are reliable. I'd also disagree that `ir_framework/tests` should be run _only_ when the framework is actually, they should also be run when its dependencies are changed, and the framework highly depends on hotspot, so to be sure that the framework didn't get broken after changes in c2, whitebox, etc, you do need run these tests more often.

I see your point. I have not considered the dependencies which can frequently change even though the framework does not. I therefore agree with you that we should run them at least from time to time. I will move everything to  `test/hotspot/jtreg/testlibrary` in a next step.

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

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