On Thu, 3 Sep 2026 13:19:16 GMT, Marc Chevalier <[email protected]> wrote:
>> Quan Anh Mai has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 103 commits: >> >> - Merge branch 'master' into unsignedDiv >> - Merge branch 'master' into unsignedDiv >> - Merge branch 'master' into unsignedDiv >> - Fix comments, remove dead code >> - Merge branch 'master' into unsignedDiv >> - Various fixes >> - Fix IntegerDivValueTests >> - Fix merge conflicts >> - Merge branch 'master' into unsignedDiv >> - Merge branch 'master' into unsignedDiv >> - ... and 93 more: https://git.openjdk.org/jdk/compare/6da8ba2d...c0ff6d16 > > Sure! I'll take a look at it soon ™. I'll start shortly, but it looks a bit > long. @marc-chevalier Thanks a lot for your reviews, I think I have addressed all of them. > src/hotspot/share/opto/divconstants.cpp line 32: > >> 30: // division by constant into a multiply/shift series. >> 31: // >> 32: // (1) Theory: > > Nit: do you mean "Theorem"? I think it goes better in the Theorem/Proof pair, > no? You are right, the original idea is Theory/Implementation, but then the other 2 sections appear :) > src/hotspot/share/opto/divconstants.cpp line 78: > >> 76: // This implies floor(x / d) = floor(x * c / m) for v + 1 <= x <= v + d >> - 1 >> 77: // >> 78: // Combining all the cases gives us the conclusion. > > It is correct, but I think it is worth explaining why that covers the whole > range. I'd rather phrase `v + 1 <= x <= v + d - 1` as simply a lower bound, > making the partition clear since the upper bound plays no role in splitting > the range for `x`. And inside this case, I'd argue that the upper bound. Same > in the `(b)` case. Done. > src/hotspot/share/opto/divconstants.cpp line 131: > >> 129: template <class T> >> 130: void magic_divide_constants(T d, T N_neg, T N_pos, juint min_s, T& c, >> bool& c_ovf, juint& s) { >> 131: static_assert(std::is_unsigned<T>::value, "calculations must be done >> in the unsigned domain"); > > Suggestion: > > static_assert(std::is_unsigned_v<T>, "calculations must be done in the > unsigned domain"); > > Nit, but that should work, right? Yes, that should work. This was before C++17 so `is_unsigned_v` was not a thing then. > src/hotspot/share/opto/divnode.cpp line 102: > >> 100: // such that division(x / d) = floor(x * c / 2**s) + (x < 0 ? 1 : 0) >> for every integer x in >> 101: // the input range. The functions in this file try to derive from the >> formula in real >> 102: // arithmetic to arrive at a formula in int/long arithmetic. More >> details can be found in > > When you say "real arithmetic", you mean "arithmetic on mathematical > integers", not "arithmetic on real numbers" (as in \mathbb{R}), right? Well, not really, the division operation is not defined on the ring of integers, so this must be arithmetic on real numbers. > src/hotspot/share/opto/divnode.cpp line 120: > >> 118: juint max_pos = dti->_hi > 0 ? juint(dti->_hi) : 0; >> 119: if (min_neg < d && max_pos < d) { >> 120: return new ConINode(TypeInt::ZERO); > > I suggest `phase->zerocon(T_INT)` or `phase->intcon(0)`. That can't be, `Ideal` must return a new node. > src/hotspot/share/opto/divnode.cpp line 151: > >> 149: // Divide-by-power-of-2 can be made into a shift, but you have to >> do >> 150: // more math for the rounding. You need to add 0 for positive >> 151: // numbers, and "i-1" for negative numbers. Example: i=4, so the > > Can we take this opportunity to fix this comment? I don't think `i` is > introduced before and if it is, I can't find it, so it wouldn't hurt to > repeat it. I remember I found it in the while and was already irritated by it. Done. > src/hotspot/share/opto/divnode.cpp line 199: > >> 197: // q = (x * c) >> s + (x < 0 ? 1 : 0) = (x * c) >> s - (x >> (W - 1)) >> 198: constexpr int N = 32; >> 199: Node* addend1 = phase->transform(new RShiftINode(dividend, >> phase->intcon(N - 1))); > > It's a bit awkward the thing noted W in the comment above is called `N` as a > variable. I think what you have been calling `N` before was the upper bound > of the domain. I think it would be consistent if you rename this variable `W`. That's right, I have renamed all of them to `W`. > src/hotspot/share/opto/divnode.cpp line 899: > >> 897: } >> 898: >> 899: // Otherwise we give up all hope > >> Lasciate ogne speranza, voi ch'intrate > > But doesn't that apply to about 90% of C2? > > (and is it on purpose you kept the comment here but not in the `I` version? > Not sure it is very useful, but it doesn't hurt to remind that's just the > ultimate fallback) Removed it. > src/hotspot/share/opto/divnode.hpp line 225: > >> 223: virtual const Type* Value(PhaseGVN* phase) const; >> 224: virtual const Type* bottom_type() const { return TypeInt::INT; } >> 225: virtual uint ideal_reg() const { return Op_RegI; } > > Why reordering that? Not that I think it was better before... For consistency with other nodes in this file only. And it looks more pleasant as `Ideal` and `Value` are closely related. ------------- PR Comment: https://git.openjdk.org/jdk/pull/31033#issuecomment-5604086639 PR Review Comment: https://git.openjdk.org/jdk/pull/31033#discussion_r3969906741 PR Review Comment: https://git.openjdk.org/jdk/pull/31033#discussion_r3969908845 PR Review Comment: https://git.openjdk.org/jdk/pull/31033#discussion_r3969914369 PR Review Comment: https://git.openjdk.org/jdk/pull/31033#discussion_r3969946988 PR Review Comment: https://git.openjdk.org/jdk/pull/31033#discussion_r3969930326 PR Review Comment: https://git.openjdk.org/jdk/pull/31033#discussion_r3969931572 PR Review Comment: https://git.openjdk.org/jdk/pull/31033#discussion_r3969933905 PR Review Comment: https://git.openjdk.org/jdk/pull/31033#discussion_r3969950765 PR Review Comment: https://git.openjdk.org/jdk/pull/31033#discussion_r3969924133
