RFR: 8332402: [IR Framework] Add tests for applyIfCPUFeature* and applyIfPlatform* in TestBadFormat

[Emanuel Peter]

On Mon, 20 May 2024 08:27:33 GMT, Hamlin Li <mli at openjdk.org> wrote:

> Hi,
> Can you help to review this patch to add some test for IR test framework?
> As discussed https://github.com/openjdk/jdk/pull/19270#pullrequestreview-2060974799, it's worth to add some tests for for applyIfCPUFeature* and applyIfPlatform* in TestBadFormat.
> 
> Thanks

Thanks for the work! Generally looks good. I have a few comments.
We should also wait a week, @chhagedorn is out of the office. He generally has more context on the IR framework.

@Hamlin-Li for https://github.com/openjdk/jdk/pull/19270

> @Hamlin-Li you only got 1 review. Per the rules, you generally need 2:
> https://openjdk.org/guide/#final-check-before-creating-the-pr
> 
> That is unless you say that the change is trivial, and the reviewer also confirms that it is trivial. I don't see that here.
> 
> Our rule is that you need 2 reviewers: at least one reviewer, the second one can be a committer.

test/hotspot/jtreg/testlibrary_tests/ir_framework/tests/TestBadFormat.java line 849:

> 847:     @Test
> 848:     @IR(failOn = IRNode.CALL, applyIf = {"TLABRefillWasteFraction", "50"}, applyIfNot = {"UseTLAB", "true"})
> 849:     @IR(failOn = IRNode.CALL, applyIfAnd = {"TLABRefillWasteFraction", "50", "UseTLAB", "true"},

Not sure why you changed the format here. You seem to have one-liners everywhere else? Plus it just creates noise when reviewing.

test/hotspot/jtreg/testlibrary_tests/ir_framework/tests/TestBadFormat.java line 861:

> 859:     public void onlyOneApplyIfCPUFeature() {}
> 860: 
> 861:     @FailCount(3)

Why are there 3 failures?

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19302#pullrequestreview-2067876106
PR Comment: https://git.openjdk.org/jdk/pull/19302#issuecomment-2122018971
PR Review Comment: https://git.openjdk.org/jdk/pull/19302#discussion_r1607858537
PR Review Comment: https://git.openjdk.org/jdk/pull/19302#discussion_r1607852163