RFR: 8205694: AArch64: Add test to validate volatile load, store and CAS code generation

[Andrew Dinn]

Hi Dmitrij,

On 26/06/18 19:33, Dmitrij Pochepko wrote:
> Overall looks good, but I have few minor comments regarding these tests:

Thanks for reviewing the patch.

> 1. Most tests which spawn other precesses for testing purposes are
> launched as "@run driver ..." for optimization reasons.

I'm using the Process API so I can retrieve and analyze the output
written to the spawned JVM's stdout. Is it possible to do that using
"@run driver"?

> 2. Do we really need to ignore external vm options set in jtreg via
> option: -javaoptions:"<additional options>"? In case these tests
> shouldn't ignore external options, there is
> ProcessTools.createJavaProcessBuilder(boolean addTestVMAndJavaOptions,
> String... command) for that.

Hmm, good question. I cannot see a need to pass on any extra test
options to the spawned JVMs at the moment. The only reason to create the
spawned process is to observe how the JIT generates code for volatile
ops and, as far as I am aware, all relevant JIT and GC options that
affect that aspect of its operation are specified in the test. However,
I may well have overlooked something.

Do you have a specific reason for thinking that certain JVM options need
to be passed on? If not perhaps we could consider passing on extra
javaoptions if/when a case for it arises.

> 3. This test will fail if jdk will be built without all GCs or without
> C2. It probably a good idea to split it in few tests with separate GC
> configurations

Well, I had thought about splitting out separate tests for some GCs,
although I was only motivated to do so for Shenandoah and ZGC -- to cope
with their potential presence/absence.

The test may also eventually need to allow for future disappearance of
CMS (currently deprecated). However, that event will also require
changes to the JIT code to make the case handling for CMS conditional on
its presence/absence -- I am inclined to postpone any such surgery on
the current test until CMS goes away.

However, I don't understand how splitting the tests up is going to help
deal with the either of the two config omissions you are concerned
about. Consider first the question of whether C2 is present. I could
specify @requires vm.flavor=="server" in the header comment but I
believe that would only indicate that "--server" has been passed as an
argument to the jtreg test jvm. That flag could be absent and the JVM
could still include a C2 compiler. However, by adding it as a
requirement the result would be that the test would only get run if the
test run explicitly passes "-server" i.e. it would effectively disable
the test.

Similarly, if I put, say, the parallel tests into a separate file I
could then add @requires vm.gc = "Parallel". However, once again I
believe that would only detect whether the jtreg test jvm had been
started using -XX:+UseParallelGC. It would not ensure that the test
would be run using a Parallel GC when some other GC was configured for
jtreg. Nor would it identify whether that a Parallel GC was available
into the JVM being tested. SO, once again it would disable all tests
(except, perhaps, those for G1).

If I have misunderstood how the @requires config works and how splitting
the tests up would improve clarity of test outcomes then I'd be happy to
be corrected here.

Also, I am not really sure how important it is to make this test work
when the config omits C2 or removes all GCs. This test is needed to make
sure that the standard AArch64 build which includes C2 and all GCs is
not broken. That's the critical case to validate. I'd prefer to leave
this as is for now unless and until jtreg makes it possible to identify
whether a specific JIT or GC is available in a JVM under test as opposed
to testing what GC is configured on the jtreg command line.

> 4. In case external options are not ignored, you'll need to filter out
> several vm options, like skipping execution for c1 or interpreter
> testruns. Something like this can be added: @requires vm.compMode !=
> "Xint" & vm.flavor == "server" & (vm.opt.TieredStopAtLevel == null |
> vm.opt.TieredStopAtLevel==4)
I don't think that is needed given that I don't intend to pass on the
jtreg test options.

regards,


Andrew Dinn
-----------
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander