On Wed, 28 Jan 2026 08:13:23 GMT, Christian Hagedorn <[email protected]> wrote:
>> Damon Fenacci has updated the pull request incrementally with one additional >> commit since the last revision: >> >> JDK-8374852: fix star layout >> >> Co-authored-by: Christian Hagedorn <[email protected]> > > src/hotspot/share/opto/opaquenode.hpp line 146: > >> 144: // builds, we keep the actual checks as additional verification code >> (i.e. removing OpaqueCheckNodes and use the >> 145: // BoolNode inputs instead). >> 146: class OpaqueCheckNode : public Node { > > I've also thought about the name. `OpaqueCheck` is already a good indication > what the node is about. Maybe we could go a step further and call it > `OpaqueConstantBoolNode` to emphasize more that it is belonging to a > `BoolNode` whose result we already know. What do you think? > > Then we could also think about changing `_positive` to `_constant` (still can > be a boolean to just pass true and false which seems more intuitive then > passing in 1 and 0). I was still had a doubt about what to put first (`Constant` or `Bool`) but I think `ConstantBool` is actually more correct. I suppose `_constant` is better than `_value` since we use it already 😉 Done. > src/hotspot/share/opto/opaquenode.hpp line 148: > >> 146: class OpaqueCheckNode : public Node { >> 147: private: >> 148: bool _positive; > > Now that we define a field, we also need to override `size_of()` (see for > example `OpaqueMultiversioningNode`). Good to know. Thanks! > src/hotspot/share/opto/opaquenode.hpp line 150: > >> 148: bool _positive; >> 149: public: >> 150: OpaqueCheckNode(Compile* C, Node* tst, bool positive) : Node(nullptr, >> tst), _positive(positive) { > > `tst` is probably almost always a `BoolNode`. I'm wondering if it could also > be a constant because we already folded the `BoolNode`? But then it's > probably also useless to create the opaque node in the first place. Hmmm... I find it hard to totally exclude a constant (e.g. if its inputs are constant...?). In that case we could skip all the opaque business (I guess in the few places where new `OpaqueConstantBool` nodes are created). On the other hand the opaque node should only really delay the folding... 🤔 ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/29164#discussion_r2737356877 PR Review Comment: https://git.openjdk.org/jdk/pull/29164#discussion_r2737353309 PR Review Comment: https://git.openjdk.org/jdk/pull/29164#discussion_r2737355777
