RFR: 8364766: C2: Improve Value() of DivI and DivL for non-constant inputs [v6]

[Tobias Hotz]

On Mon, 25 Aug 2025 12:26:02 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Tobias Hotz has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Remove too strict assert from old code path
>
> src/hotspot/share/opto/divnode.cpp line 508:
> 
>> 506: 
>> 507: template<typename IntegerType>
>> 508: static const IntegerType* compute_generic_div_type(const IntegerType* i1, const IntegerType* i2, int widen) {
> 
> Do we need the `generic` in the name? The `template` already suggests that it can be used for different types, right?
> 
> Also: I'm wondering if we can somehow extend this for `UDivI` and `UDIvL`.
> I suppose you would have to use the `_ulo` and `_uhi` instead of `_lo` and `_hi`.
> 
> I'm not saying this all has to be done in this PR, but we could at least anticipate the extension to unsigned division.

I've adjusted it to `compute_signed_div_type`.
I would not worry about unsigned division now and would leave this for a later RFE. Would you perhaps create a issue for that? I have no permission for the JBS

> src/hotspot/share/opto/divnode.cpp line 533:
> 
>> 531:   // Here i2 is entirely negative or entirely positive.
>> 532:   // Let d_min and d_max be the nonzero endpoints of i2.
>> 533:   // Then a/b is monotonic in a and in b (when b keeps the same sign).
> 
> I think you should talk about `i1` and `i2`. You have not defined `a` and `b` up to now.

Yeah, fixed

> src/hotspot/share/opto/divnode.cpp line 547:
> 
>> 545:   // Special overflow case: min_val / (-1) == min_val (cf. JVMS§6.5 idiv/ldiv)
>> 546:   // We need to be careful that we never run min_val / (-1) in C++ code, as this overflow is UB there
>> 547:   // We also must include min_val in the output if i1->_lo == min_val and i2->_hi.
> 
> `if i1->_lo == min_val and i2->_hi` I cannot parse this.
> The `if` suggests that there will be a condition following. The `and` confirms that.
> Then I see `i1->_lo == min_val` which is a boolean condition. But `i2->_hi` is not.
> Ah, did you mean this the condition from below?
> Suggestion:
> 
>   // We also must include min_val in the output if i1->_lo == min_val and i2->_hi == -1.

I've stripped this line entirely. It doesn't make much sense in this version and is a leftover

> src/hotspot/share/opto/divnode.cpp line 552:
> 
>> 550:     NativeType new_lo = min_val;
>> 551:     NativeType new_hi;
>> 552:     // compute new_hi for non-constant divisor and/or dividend.
> 
> You suggest we only land here in non-constant cases. Is that true?
> What if `i1=min_val` and `i2=-1`?

Yeah this comment was outdated. I've reworded it.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26143#discussion_r2443238083
PR Review Comment: https://git.openjdk.org/jdk/pull/26143#discussion_r2443237457
PR Review Comment: https://git.openjdk.org/jdk/pull/26143#discussion_r2443237301
PR Review Comment: https://git.openjdk.org/jdk/pull/26143#discussion_r2443237018