RFR: 8345766: C2 should emit macro nodes for ModF/ModD instead of calls during parsing

[Emanuel Peter]

On Wed, 8 Jan 2025 07:56:22 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> src/hotspot/share/opto/callnode.cpp line 721:
>> 
>>> 719: const Type *CallNode::bottom_type() const { return tf()->range(); }
>>> 720: const Type* CallNode::Value(PhaseGVN* phase) const {
>>> 721:   if (!in(0) || phase->type(in(0)) == Type::TOP) {
>> 
>> We use explicit compare `in(0) == nullptr`.
>
> Suggestion:
> 
>   if (in(0) == nullptr || phase->type(in(0)) == Type::TOP) {
> 
> 
> `Do not use ints or pointers as (implicit) booleans with &&, ||, if, while. Instead, compare explicitly, i.e. if (x != 0) or if (ptr != nullptr), etc.`
> 
> See
> https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md

Why can this now be nullptr?

>> src/hotspot/share/opto/parse2.cpp line 1100:
>> 
>>> 1098: 
>>> 1099: Node* Parse::floating_point_mod(Node* a, Node* b, bool dbl) {
>>> 1100:   CallNode* mod = dbl ? static_cast<CallNode*>(new ModDNode(C, a, b)) : new ModFNode(C, a, b);
>> 
>> Why you need the case for `ModDNode`?
>
> He has a call from `Bytecodes::_frem:` and from `Bytecodes::_drem:`.
> 
> Why not make it a `BasicType bt` instead of `dbl`, and then switch on that? Might be more readable than true / false.
> I read `floating_point_mod(a, b, true)`, and am not sure what the `true` does.

Why do you need the `static_cast<CallNode*>`? I mean why not use the common type `ModFloatingNode*`, which is a subtype of `CallNode*`, right?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22786#discussion_r1906656119
PR Review Comment: https://git.openjdk.org/jdk/pull/22786#discussion_r1906716363