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

[Igor Ignatyev]

On Mon, 19 Apr 2021 11:11:40 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> test/lib/jdk/test/lib/hotspot/ir_framework/AbstractInfo.java line 57:
>> 
>>> 55:      * @return an inverted boolean of the result of the last invocation of this method.
>>> 56:      */
>>> 57:     public boolean toggleBoolean() {
>> 
>> I don't think `toggleBoolean` really belongs to `AbstractInfo`, I'd rather move it into a (separate) aux class.
>
> The problem with having a separate class for it is that we would need to create a new instance somehow in a test and store it in a field to access it in subsequent iterations of a test. On the other hand, one could argue that we do not really need to guarantee that the first invocation of `toggleBoolean()` always returns `false` as the common use-case of this method is just to execute some if-else paths (roughly) equally often (regardless with which one we start) . Then we could also just move the method to `TestFramework::toggleBoolean()` with a static boolean field. We are always only executing a single test at a time, so `toggleBoolean()` reliably returns a different boolean each time it is called in a single test. What do you think?

`TestFramework::toggleBoolean()` sounds like a good alternative.

>> test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 107:
>> 
>>> 105:      */
>>> 106:     public static final Set<String> JTREG_WHITELIST_FLAGS = new HashSet<>(
>>> 107:             Arrays.asList(
>> 
>> it doesn't seem to be a comprehensive list of flags that don't affect IR verification work, e.g. different GCs. I think it might be easy to go with the blacklist instead, and possibly warning people if there are any flags in `test.*.opts`.
>
> Maybe first some general background about having a whitelist/blacklist in the first place. When a user writes an `@IR` rule, we do not want him to think about any possible VM flag combination (which he is not specifically using in his test, for example with `addFlags`) that could break his `@IR` rule and then requiring him to restrict the `@IR` rule with `IR::applyIfXX()` for these flags.
> 
> I agree that the whitelist is probably not complete and could be improved (but I think we should not whitelist different GCs as these affect the barrier code of the IR). About whitelist vs. blacklist, I had discussions about it with @TobiHartmann. We eventually decided to use a whitelist which seemed easier to maintain and is probably a lot shorter than a blacklist. In addition, if a new flag is introduced or removed, we most likely do not need to worry about updating the whitelist as the (currently) whitelisted flags will most likely remain unchanged. This first whitelist version is also just a best guess solution. We might miss on some other flags that will not affect the IR but we have the confidence that we do not run into surprising failures due to forgetting to blacklist a particular flag. But this is open for discussion of course. It would be interesting to hear how people think about it.

I'll need to think about it a bit more, but my first impression after reading your comment is that we don't actually want to allow any flags, so we might better off just using `@requies vm.flagless` or reusing the underlying code to check that we don't have any flags that can "significantly" change behavior.

> But I think most of the time, the user wants to test the calling class for which this additional constructor is quite useful.

I concur that in most cases the users would want to use the caller as a `testClass`, yet we already have `run` and friends to cover that (most common) use-case, users who would need to construct fancier `TestFramework`, most probably, won't be doing it in their `testClass`, and if they would, the overhead of writing `new TestFramework(getClass())` is just 10 keystrokes and is neglatible (given the assumption of a more complicated use case)

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

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