On Wed, 8 Mar 2023 19:38:56 GMT, Vladimir Ivanov <vliva...@openjdk.org> wrote:
>> Vladimir Kozlov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove RISC-V port code for float16 intrinsics
>
> src/hotspot/share/opto/convertnode.cpp line 171:
>
>> 169: if (t == Type::TOP) return Type::TOP;
>> 170: if (t == Type::FLOAT) return TypeInt::SHORT;
>> 171: if (StubRoutines::f2hf() == nullptr) return bottom_type();
>
> What's the purpose of this check? My understanding is ConvF2HF/ConvHF2F
> require intrinsification and on platforms where stubs are absent,
> intrinsification is disabled.
This code is optimization: use stub to calculate constant value during
compilation instead of generating HW instruction in compiled code. It is not
required to have this stub for intensification to work - `ConvF2HFNode` will be
processed normally and will use intrinsics code (HW instruction) defined in .ad
file.
These stubs are used only here, not in C1 and not in Interpreter. As
consequence these stubs implementation is optional and I implemented them only
on x64. That is why I have this check.
I debated to not have them at all to not confuse people but they did improve
performance a little.
> src/hotspot/share/opto/convertnode.cpp line 244:
>
>> 242:
>> 243: const TypeInt *ti = t->is_int();
>> 244: if (ti->is_con()) {
>
> I find it confusing that `ConvHF2FNode::Value()` has `is_con()` check, but
> `ConvF2HFNode::Value()`doesn't. I'd prefer to see both implementations
> unified.
It follows the same pattern as other nodes here: `ConvF2INode::Value()` vs
`ConvI2FNode::Value()`.
If you want to change it we need to do that in separate RFE for all methods
here.
But I don't think we need to do that because Float/Double does not have range
values as Integer types.
Float have only 3 types of value: FloatTop, FloatBot, FloatCon. So we don't
need to check for constant if checked for TOP and BOT. For Integer we need to
check `bool is_con() const { return _lo==_hi; }`.
-------------
PR: https://git.openjdk.org/jdk/pull/12869
