RFR: 8319793: C2 compilation fails with "Bad graph detected in build_loop_late" after JDK-8279888 [v9]

[Emanuel Peter]

On Thu, 4 Jan 2024 16:09:05 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Roland Westrelin has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 11 commits:
>> 
>>  - Merge branch 'master' into JDK-8319793
>>  - review
>>  - Revert "Update src/hotspot/share/opto/castnode.hpp"
>>    
>>    This reverts commit 356c91cca911ed486f9f87f3eff53ce21e1e3ec9.
>>  - Revert "Update src/hotspot/share/opto/memnode.hpp"
>>    
>>    This reverts commit bdb731ea562f314f44d327f7243ef5cf9ad40b2e.
>>  - review
>>  - Update src/hotspot/share/opto/memnode.hpp
>>    
>>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>>  - Update src/hotspot/share/opto/castnode.hpp
>>    
>>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>>  - Update src/hotspot/share/opto/castnode.cpp
>>    
>>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>>  - Update src/hotspot/share/opto/ifnode.cpp
>>    
>>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>>  - Merge branch 'master' into JDK-8319793
>>  - ... and 1 more: https://git.openjdk.org/jdk/compare/15519285...dbe3c4c1
>
> src/hotspot/share/opto/ifnode.cpp line 1958:
> 
>> 1956:         return nullptr;
>> 1957:       }
>> 1958: 
> 
> Why is it ok to delay this to post-loop-opts? Does it not prevent moving some CFG from being eliminated out of loops? Would be nice to have a little justification comment.

Ah. Does this mean that if there are multiple RangeCheck in a loop, where some could be smeared, these are not smeared, and then we have more RangeChecks to eliminate out of the loop? Maybe in the end this all comes down to the same anyway. Just wondering.

> src/hotspot/share/opto/node.hpp line 1140:
> 
>> 1138:   virtual Node* pin_for_array_access() const {
>> 1139:     return nullptr;
>> 1140:   }
> 
> Can you please add a comment, what this method is for?

Effectively, you want to replace some nodes, such as `Load` and `CastII` into pinned nodes, which have `StrongDependency` or `UnknownControl`. In either case, this means that we will not allow these to float any more.

Generally, I'm not really happy with the name of `UnknownControl`. Sounds like the control is unknown. In what sense is it unknown, after all we have a control and want the Load to be pinned to it...?

Maybe then we could rename `pin_for_array_access` -> `make_pinned`.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1441954551
PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1441907080