This is an archive of the discontinued LLVM Phabricator instance.

clang: Emit nofpclass(nan inf) for -ffinite-math-only
ClosedPublic

Authored by arsenm on Mar 2 2023, 3:00 AM.

Details

Summary

Set this on any source level floating-point type argument,
return value, call return or outgoing parameter which is lowered
to a valid IR type for the attribute. Currently this isn't
applied to emitted intrinsics since those don't go through
ABI code.

Diff Detail

Event Timeline

arsenm created this revision.Mar 2 2023, 3:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2023, 3:00 AM
arsenm requested review of this revision.Mar 2 2023, 3:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2023, 3:00 AM
jcranmer-intel added inline comments.Mar 2 2023, 7:48 AM
clang/lib/CodeGen/CGCall.cpp
2212

Clang doesn't have support for -f[no-]signaling-nans yet, but the gcc documentation for the option states:

Compile code assuming that IEEE signaling NaNs may generate user-visible traps during floating-point operations. Setting this option disables optimizations that may change the number of exceptions visible with signaling NaNs. This option implies -ftrapping-math.

This strikes me as saying that sNaNs are treated as qNaN (akin to the nsz fast-math flag) rather than saying that it's UB to have sNaN as a value, thus, I don't think it makes sense for -fno-signaling-nans to translate into a nofpclass attribute.

arsenm updated this revision to Diff 502015.Mar 2 2023, 3:32 PM

Drop todo

nikic resigned from this revision.Mar 3 2023, 12:22 AM

(Sounds reasonable conceptually, but I'm not familiar enough with clang codegen for the details.)

I'm generally okay with the approach of this patch. I'm not sufficiently well-versed in the clang codegen side of things to know if this covers all of the bases, and I'd appreciate someone who is familiar with that side of things to approve this patch.

tra added a subscriber: nikic.Mar 13 2023, 10:11 AM

I'm in the same boat as @nikic -- the patch looks reasonable in principle, but I'm not familiar with the nuanaces of FP codegen.

Looks fine from a codegen perspective, assuming these are the semantics we want for -ffinite-math-only.

clang/lib/CodeGen/CGCall.cpp
3053

What cases does this cover that aren't already covered by the code in CodeGenModule::ConstructAttributeList?

arsenm updated this revision to Diff 505253.Mar 14 2023, 2:03 PM
arsenm marked an inline comment as done.

Drop unnecessary part

This revision is now accepted and ready to land.Mar 14 2023, 4:08 PM
arsenm closed this revision.Mar 14 2023, 10:13 PM
clang/lib/CodeGen/CGCall.cpp
3053

Nothing apparently