RFR: 8335392: C2 MergeStores: enhanced pointer parsing [v13]

[Emanuel Peter]

On Fri, 1 Nov 2024 12:25:43 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   apply more suggestions from Christian
>
> src/hotspot/share/opto/mempointer.hpp line 389:
> 
>> 387: 
>> 388: public:
>> 389:   MemPointerAliasing() : MemPointerAliasing(Unknown, 0) {}
> 
> Does not look like you call this constructor directly. You can therefore make it private as well:
> Suggestion:
> 
>   MemPointerAliasing() : MemPointerAliasing(Unknown, 0) {}
>   
> public:

I just removed this constructor!

> src/hotspot/share/opto/mempointer.hpp line 393:
> 
>> 391:   static MemPointerAliasing make_unknown() {
>> 392:     return MemPointerAliasing();
>> 393:   }
> 
> Thinking about the comment above again, you can probably just remove the no-arg-constructor and simply do the following which I think is expressive enough:
> Suggestion:
> 
>   static MemPointerAliasing make_unknown() {
>     return MemPointerAliasing(Unknown, 0);
>   }

Yes, this seems better, I'm doing this :)

> src/hotspot/share/opto/mempointer.hpp line 429:
> 
>> 427:       _variable(nullptr),
>> 428:       _scale(NoOverflowInt::make_NaN()) {}
>> 429:   MemPointerSummand(Node* variable, const NoOverflowInt scale) :
> 
> Can `scale` be passed as const reference? You will make a copy anyway when assigning it to `_scale`. The compiler would probably optimize this anyway but I guess it does not hurt to use a reference here directly.

Will do that, and similarly elsewhere!

> src/hotspot/share/opto/mempointer.hpp line 438:
> 
>> 436: 
>> 437:   Node* variable() const { return _variable; }
>> 438:   NoOverflowInt scale() const { return _scale; }
> 
> Not sure if you really require to create a new object here or if you could just pass it by const reference. The usages are only in `parse_decomposed_form()`. There you either add it together, from which you create a new `NoOverFlowInt` anyway, or you use it to create a new `MemPointerSummand` which will create it's own `scale` copy anyway. But maybe I'm also missing something here.

Passing out constant references makes me a little nervous, honestly. What if the MemPointer does not outlive the use of the reference outside? I think a creation of a `NoOverflowInt` is very very cheap, and not really worth that risk...

> src/hotspot/share/opto/noOverflowInt.hpp line 57:
> 
>> 55:   bool is_zero() const { return !is_NaN() && value() == 0; }
>> 56: 
>> 57:   friend NoOverflowInt operator+(const NoOverflowInt a, const NoOverflowInt b) {
> 
> Is it required to pass the arguments by value for the overloaded operators or would it be sufficient to pass them by reference (i.e. `const NoOverflowInt& a, const NoOverflowInt& b`)?

Good idea!

> src/hotspot/share/opto/noOverflowInt.hpp line 90:
> 
>> 88: 
>> 89:   NoOverflowInt abs() const {
>> 90:     if (is_NaN()) { return make_NaN(); }
> 
> Why do you require a new `NaN` here and not simply return `*this`?

Yes, I changed it!

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19970#discussion_r1825890781
PR Review Comment: https://git.openjdk.org/jdk/pull/19970#discussion_r1825891236
PR Review Comment: https://git.openjdk.org/jdk/pull/19970#discussion_r1825892269
PR Review Comment: https://git.openjdk.org/jdk/pull/19970#discussion_r1825894476
PR Review Comment: https://git.openjdk.org/jdk/pull/19970#discussion_r1825889312
PR Review Comment: https://git.openjdk.org/jdk/pull/19970#discussion_r1825890329