This is an archive of the discontinued LLVM Phabricator instance.

[flang] Implement isnan and ieee_is_nan intrinsics
ClosedPublic

Authored by DavidTruby on Feb 23 2023, 8:28 AM.

Details

Summary

To implement these we call the LLVM intrinsic is.fpclass indicating that
we are checking for either a quiet or signalling NaN.

Diff Detail

Event Timeline

DavidTruby created this revision.Feb 23 2023, 8:28 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
Herald added a subscriber: mehdi_amini. · View Herald Transcript
DavidTruby requested review of this revision.Feb 23 2023, 8:28 AM

Clarify in unit test that in the f64 case we convert to f32 first

vzakhari added inline comments.Feb 23 2023, 9:42 AM
flang/lib/Optimizer/Builder/IntrinsicCall.cpp
3708

Could you please use mlir::LLVM::IsFPClass operation when mathRuntimeVersion != preciseVersion?

flang/test/Lower/Intrinsics/isnan.f90
39

Where is this conversion coming from? Can we get rid of it?

Clarify in unit test that in the f64 case we convert to f32 first

Is converting to f32 correct? I see that the f64 case is supported (https://github.com/llvm/llvm-project/blob/bb588519c5b5a6555e418259bb5d4a37d35f5837/llvm/test/CodeGen/X86/is_fpclass.ll#L649).

Can you add the following to the summary?
-> The isnan extension is already captured in the Extensions documentation (https://github.com/llvm/llvm-project/blob/main/flang/docs/Extensions.md).
-> Add Fixes #6082 ( https://github.com/llvm/llvm-project/issues/60882).

Could you add tests for kind=10 and 16 as well?

tschuett added a subscriber: tschuett.EditedFeb 23 2023, 1:51 PM

According to the tests in https://reviews.llvm.org/D112025 it also supports f64.

DavidTruby added inline comments.Feb 23 2023, 3:25 PM
flang/lib/Optimizer/Builder/IntrinsicCall.cpp
3708

I think we can just use mlir::LLVM::IsFPClass unconditionally? It will always lower directly to this function call but is cleaner code anyway.

flang/test/Lower/Intrinsics/isnan.f90
39

I didn't add this conversion here, it appears to be happening before the IntrinsicCall class gets hold of it, so I guess somewhere prior we're defining isnan to have that conversion. I didn't really consider it as this will be correct (converting a f64 to f32 will preserve the isnan) but on second thought we should try and avoid the conversion to remove the extra instruction. I'll see if I can find out where it is coming from.

tschuett added inline comments.Feb 24 2023, 1:02 AM
flang/lib/Optimizer/Builder/IntrinsicCall.cpp
3697

Can you check whether args are f32 or f64?

jeanPerier added inline comments.Feb 24 2023, 1:38 AM
flang/test/Lower/Intrinsics/isnan.f90
39

It is indeed inserted before lowering, probably when resolving the intrinsic call (it appears in the -fdebug-unparse).
The convert is being inserted here to simplify its processing: https://github.com/llvm/llvm-project/blob/e4c4841e5bbaf6d09aff65387102ed22a9eac60b/flang/lib/Evaluate/fold-logical.cpp#L162 (It is implicitly added because of the "DefaultReal" on FoldElementalIntrinsic that inserts conversions from the actual arguments to the requested template argument types if needed).

Remove unnecessary casts to f32 for other real types

DavidTruby added inline comments.Feb 24 2023, 6:59 AM
flang/lib/Evaluate/fold-logical.cpp
163 ↗(On Diff #500190)

I've tried to add a check that we only replace the type of the function when we are actually able to fold, i.e. when the argument can be treated as a constant real of kind 4. This seems to pass both my own tests and the existing fold tests for isnan, but I'm not 100% confident it's correct so wanted to highlight this to get a review from someone more familiar with the folding code.

DavidTruby added inline comments.Feb 24 2023, 7:02 AM
flang/test/Lower/Intrinsics/isnan.f90
39

This is now slightly confusingly pointing at a different conversion (the one to fir.logical<4>), which I don't think can be removed. Just for reference to anyone coming along to review in future.

jeanPerier added inline comments.Feb 24 2023, 7:45 AM
flang/lib/Evaluate/fold-logical.cpp
163 ↗(On Diff #500190)

I think this will prevent folding of isnan with constant arguments that are not of default real type.

It is probably best to only use args[0] && args[0]->UnwrapExpr() && IsActuallyConstant(*args[0]->UnwrapExpr()) that will let let isnan be folded for any real type constant argument.

Otherwise, bypassing the folding that cannot succeed seems OK to me.

Use suggested check for constant argument instead

vzakhari accepted this revision.Feb 27 2023, 11:18 PM
vzakhari added inline comments.
flang/lib/Optimizer/Builder/IntrinsicCall.cpp
3708

I think we can just use mlir::LLVM::IsFPClass unconditionally? It will always lower directly to this function call but is cleaner code anyway.

I think it may be okay to always generate the intrinsic operation. Thanks!

This revision is now accepted and ready to land.Feb 27 2023, 11:18 PM
This revision was automatically updated to reflect the committed changes.

FYI I see couple of gfortran tests related to isnan are failing

maxlocval_2
maxlocval_4
minlocval_1
minlocval_4
namelist_42
namelist_43
nan_6
nearest_3

FYI I see couple of gfortran tests related to isnan are failing

maxlocval_2
maxlocval_4
minlocval_1
minlocval_4
namelist_42
namelist_43
nan_6
nearest_3

An initial investigation by @SBallantyne revealed that these failures are not related to the implementation of the isnan intrinsic. See details in https://github.com/llvm/llvm-project/issues/61286#issuecomment-1462224026. Will need input before we can make progress.