RFR: 8320725: C2: Add "requires_strict_order" flag for floating-point add-reduction [v2]

[Bhavana Kilambi]

On Wed, 28 Feb 2024 08:32:52 GMT, Guoxiong Li <gli at openjdk.org> wrote:

>> Bhavana Kilambi has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Addressed review comments for changes in backend rules and code style
>
> src/hotspot/cpu/aarch64/aarch64_vector.ad line 2891:
> 
>> 2889:   predicate((!VM_Version::use_neon_for_vector(Matcher::vector_length_in_bytes(n->in(2))) &&
>> 2890:              !n->as_Reduction()->requires_strict_order()) ||
>> 2891:             n->as_Reduction()->requires_strict_order());
> 
> This predication looks strange and complex. Can it be simplified to `!VM_Version::use_neon_for_vector(Matcher::vector_length_in_bytes(n->in(2))) || n->as_Reduction()->requires_strict_order()`?

Hi, thanks for the suggestion. I agree it is a bit cumbersome but I felt it's easier to understand the various conditions on which these SVE instructions can be generated. Nevertheless, the suggested changes feel more compact. I made the changes in the new PS.

> src/hotspot/share/opto/vectorIntrinsics.cpp line 1740:
> 
>> 1738:   Node* value = nullptr;
>> 1739:   if (mask == nullptr) {
>> 1740:     assert(!is_masked_op, "Masked op needs the mask value never null");
> 
> This assert may be missed after your refactor. But it seems not really matter.

Yes, the conditions of `mask != nullptr` should take care of that.

> src/hotspot/share/opto/vectornode.hpp line 242:
> 
>> 240:   virtual bool requires_strict_order() const {
>> 241:     return false;
>> 242:   };
> 
> The last semicolon is redundant.

Done

> src/hotspot/share/opto/vectornode.hpp line 265:
> 
>> 263: class AddReductionVFNode : public ReductionNode {
>> 264: private:
>> 265:   bool _requires_strict_order; // false in Vector API.
> 
> The comment `false in Vector API` seems not so clean. We need to state the meaning of the field instead of one of its usages?

Done

> src/hotspot/share/opto/vectornode.hpp line 276:
> 
>> 274: 
>> 275:   virtual bool cmp(const Node& n) const {
>> 276:     return Node::cmp(n) && _requires_strict_order== ((ReductionNode&)n).requires_strict_order();
> 
> Need a space before `==`

Done

> src/hotspot/share/opto/vectornode.hpp line 297:
> 
>> 295: 
>> 296:   virtual bool cmp(const Node& n) const {
>> 297:     return Node::cmp(n) && _requires_strict_order== ((ReductionNode&)n).requires_strict_order();
> 
> Need a space before `==`

Done

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18034#discussion_r1512315765
PR Review Comment: https://git.openjdk.org/jdk/pull/18034#discussion_r1512318336
PR Review Comment: https://git.openjdk.org/jdk/pull/18034#discussion_r1512319129
PR Review Comment: https://git.openjdk.org/jdk/pull/18034#discussion_r1512319002
PR Review Comment: https://git.openjdk.org/jdk/pull/18034#discussion_r1512318763
PR Review Comment: https://git.openjdk.org/jdk/pull/18034#discussion_r1512318536