[lworld] RFR: 8235914: [lworld] Profile acmp bytecode [v3]

[Tobias Hartmann]

On Fri, 9 Oct 2020 12:00:35 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> This includes:
>> - a new ProfileData structure to profile both inputs to acmp
>> - profile collection at acmp in the interpreter on x86
>> - profile collection at acmp in c1 generated code on x86
>> - changes to c2's acmp implementation to leverage profiling (both existing profiling through type speculation and new
>>   profile data at acmp)
>> - small tweaks to the assembly code generated for acmp
>> - a change to the implementation of LIRGenerator::profile_null_free_array() so it doesn't use a branch (which is
>>   dangerous given the register allocator is not aware of branches added at the LIR level)
>> - new tests
>> 
>> Profile collection happens unconditionally. Leveraging profiling at acmp is under UseACmpProfile which is false by
>> default.
>
> Roland Westrelin has updated the pull request with a new target base due to a merge or a rebase. The pull request now
> contains two commits:
>  - fix for CI test failures
>  - acmp profiling

Changes requested by thartmann (Committer).

test/hotspot/jtreg/compiler/valhalla/inlinetypes/InlineTypeTest.java line 239:

> 237:     protected static final String JLONG_ARRAYCOPY = "(.*call_leaf_nofp,runtime  jlong_disjoint_arraycopy.*" + END;
> 238:     protected static final String FIELD_ACCESS = "(.*Field: *" + END;
> 239:     protected static final String SUBSTITUTABLITY_TEST = START + "CallStaticJava" + MID +
> "java.lang.invoke.ValueBootstrapMethods::isSubstitutable" + END;

`SUBSTITUTABLITY_TEST` -> `SUBSTITUTABILITY_TEST`

test/hotspot/jtreg/compiler/valhalla/inlinetypes/InlineTypeTest.java line 300:

> 298:                 "-XX:+StressInlineTypeReturnedAsFields"};
> 299:         case 3: return new String[] {
> 300:                 "-XX:-UseACmpProfile",

We can enable profiling when IR verification is disabled `-DVerifyIR=false`.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/InlineTypeTest.java line 309:

> 307:                 "-XX:+InlineTypeReturnedAsFields"};
> 308:         case 4: return new String[] {
> 309:                 "-XX:-UseACmpProfile",

We can enable profiling when IR verification is disabled `-DVerifyIR=false`.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestLWorld.java line 3388:

> 3386:     }
> 3387:
> 3388:     // acmp doesn't need substitutablity test when one input is known

"substitutablity" -> "substitutability"

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestLWorld.java line 3401:

> 3399:     }
> 3400:
> 3401:     // acmp doesn't need substitutablity test when one input null

"substitutablity" -> "substitutability"

src/hotspot/cpu/x86/templateTable_x86.cpp line 2500:

> 2498:     __ andptr(rbx, Address(rax, oopDesc::mark_offset_in_bytes()));
> 2499:     __ andptr(rbx, is_inline_type_mask);
> 2500:     __ cmpptr(rbx, is_inline_type_mask);

Good catch!

src/hotspot/share/c1/c1_Instruction.hpp line 2730:

> 2728:   Value right()                 const { return _right; }
> 2729:   bool left_maybe_null()       const { return _left_maybe_null; }
> 2730:   bool right_maybe_null()       const { return _right_maybe_null; }

Indentation looks weird.

src/hotspot/share/oops/methodData.hpp line 397:

> 395:   virtual bool is_SpeculativeTrapData()const { return false; }
> 396:   virtual bool is_ArrayLoadStoreData() const { return false; }
> 397:   virtual bool is_ACmpData()          const { return false; }

Indentation is wrong.

src/hotspot/share/opto/c2_globals.hpp line 784:

> 782:                                                                             \
> 783:   product(bool, UseACmpProfile, false,                                      \
> 784:           "Take advantage of profiling at acmp")                            \

Why not enable it by default?

src/hotspot/share/opto/parse2.cpp line 2180:

> 2178: }
> 2179:
> 2180: void Parse::do_acmp(BoolTest::mask btest, Node* right, Node* left) {

`Node* left, Node* right` seems more natural. Also in the code below, where `right` is processed first.

src/hotspot/share/opto/parse2.cpp line 2313:

> 2311:   }
> 2312:   if (!left_inline_type) {
> 2313:     // Comparison with an object of known not to be an inline type

Should be "object ~~of~~ known"

src/hotspot/share/opto/parse2.cpp line 2318:

> 2316:   }
> 2317:   if (!right_inline_type) {
> 2318:     // Comparison with an object of known not to be an inline type

Should be "object ~~of~~ known"

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

PR: https://git.openjdk.java.net/valhalla/pull/185