[jdk8u-dev] RFR: 8303466: C2: failed: malformed control flow. Limit type made precise with MaxL/MinL

[Francisco Ferrari Bihurriet]

On Mon, 1 Jul 2024 20:01:46 GMT, Roman Marchenko <rmarchenko at openjdk.org> wrote:

>> Hi,
>> 
>> I would like to propose a backport of 8303466 [1] to jdk8u. jdk8u can benefit from this fix and having more accurate limit type information.
>> 
>> This pull request contains a backport of commit [8578e12c423ed61618e0b3ef81e5be3d18be1da2](https://github.com/openjdk/jdk11u-dev/commit/8578e12c423ed61618e0b3ef81e5be3d18be1da2) from the [openjdk/jdk11u-dev](https://github.com/openjdk/jdk11u-dev) repository.
>> 
>> This backport depends on [JDK-8262017](https://bugs.openjdk.org/browse/JDK-8262017).
>> 
>> The jdk11u patch does not apply cleanly because of the following:
>> 
>>  * hotspot/src/share/vm/opto/addnode.hpp
>>    * 8u does not have 8212043. Manually applied the change.
>> 
>>  * hotspot/src/share/vm/runtime/vmStructs.cpp
>>    * 8u does not have 8212043. Manually applied the change.
>> 
>>  * hotspot/src/share/vm/opto/macro.cpp
>>    * 8u does not have 8204210. Manually applied the change.
>>    * 8u does not have 8186027. Manually applied the change.
>> 
>>  * hotspot/src/share/vm/opto/loopTransform.cpp
>>    * 8u does not have 8223142. In addition, 8u does not have 8034812. Manually applied the change.
>>    * 8u does not have 8182299. Manually applied the change.
>> 
>> In addition, the following changes were made:
>> 
>>  * Nodes allocation follows the pattern "new (C) ..." instead of "new ...".
>> 
>>  * File locations were adjusted.
>> 
>>  * In addnode.cpp, 8u does not have 8284358 so the function "Node::set_req_X(uint i, Node *n, PhaseGVN *gvn)" is not available. Picked this function (declaration and definition) from 8284358.
>> 
>>  * In 8u, the jtreg VM prop vm.compiler2.enabled does not exist. C2 will likely be available and if the test were executed without C2, it should be harmless.
>> 
>> No regressions observed in hotspot:tier1.
>> 
>> Thanks,
>> Martin.-
>> 
>> --
>> [1] - https://bugs.openjdk.org/browse/JDK-8303466
>
> hotspot/src/share/vm/opto/addnode.hpp line 295:
> 
>> 293:   virtual const Type* bottom_type() const { return TypeLong::LONG; }
>> 294:   virtual uint ideal_reg() const { return Op_RegL; }
>> 295:   virtual Node* Identity(PhaseGVN* phase);
> 
> Is it OK that `MaxLNode::Identity` has different signature than `AddNode::Identity`?
> 
>   virtual Node *Identity( PhaseTransform *phase );
> 
> Tha same question is for MinLNode and ConvI2LNode.

Yes, as [JDK-8146629](https://bugs.openjdk.org/browse/JDK-8146629 "Make phase->is_IterGVN() accessible from Node::Identity and Node::Value") (openjdk/jdk11u at 69b52aa28be9054ea53bf9587ef7b563a658e510) doesn't have a 8u backport, it would have been neater to have `PhaseTransform* phase`.

What happened here is that neither the patch application nor the compilation failed, since `PhaseGVN` inherits from `PhaseValues`, which inherits from `PhaseTransform`.

However, there shouldn't be any issue, as the C++ type system is ensuring that all the callers are passing a `PhaseGVN` pointer.

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

PR Review Comment: https://git.openjdk.org/jdk8u-dev/pull/529#discussion_r1661571987