This is an archive of the discontinued LLVM Phabricator instance.

IR: Add nofpclass parameter attribute
ClosedPublic

Authored by arsenm on Dec 12 2022, 6:34 PM.

Details

Summary

This carries a bitmask indicating forbidden floating-point value kinds
in the argument or return value. This will enable interprocedural
-ffinite-math-only optimizations. This is primarily to cover the
no-nans and no-infinities cases, but also covers the other floating
point classes for free. Textually, this provides a number of names
corresponding to bits in FPClassTest, e.g.

call nofpclass(nan inf) @must_be_finite()
call nofpclass(snan) @cannot_be_snan()

This is more expressive than the existing nnan and ninf fast math
flags. As an added bonus, you can represent fun things like nanf:

declare nofpclass(inf zero sub norm) float @only_nans()

Compared to nnan/ninf:

  • Can be applied to individual call operands as well as the return value
  • Can distinguish signaling and quiet nans
  • Distinguishes the sign of infinities
  • Can be safely propagated since it doesn't imply anything about other operands.
  • Does not apply to FP instructions; it's not a flag

This is one step closer to being able to retire "no-nans-fp-math" and
"no-infs-fp-math". The one remaining situation where we have no way to
represent no-nans/infs is for loads (if we wanted to solve this we
could introduce !nofpclass metadata, following along with
noundef/!noundef).

This is to help simplify the GPU builtin math library
distribution. Currently the library code has explicit finite math only
checks, read from global constants the compiler driver needs to set
based on the compiler flags during linking. We end up having to
internalize the library into each translation unit in case different
linked modules have different math flags. By propagating known-not-nan
and known-not-infinity information, we can automatically prune the
edge case handling in most functions if the function is only reached
from fast math uses.

Diff Detail

Event Timeline

arsenm created this revision.Dec 12 2022, 6:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 12 2022, 6:34 PM
arsenm requested review of this revision.Dec 12 2022, 6:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 12 2022, 6:34 PM
Herald added a subscriber: wdng. · View Herald Transcript
nikic added inline comments.Dec 19 2022, 9:57 AM
llvm/docs/LangRef.rst
1409

The wording here is unusual -- we'd normally just say that the argument/return is poison. Why the focus on the using operation here?

1424

I don't really get what this whole paragraph is trying to say.

Please at least write out the 0xf0.

pengfei added inline comments.
clang/lib/CodeGen/CGBuiltin.cpp
14781 ↗(On Diff #482330)

What's broken here? Intrinsics are usually mapped directly to instructions which have different assumptions on the values.

arsenm added inline comments.Dec 20 2022, 5:35 AM
clang/lib/CodeGen/CGBuiltin.cpp
14781 ↗(On Diff #482330)

Accidental diff. This is forcibly setting fast math flags on the builtins regardless of the fp mode, and it's not using a flag guard. This is going to infect later built instructions with wrong fast math flags

pengfei added inline comments.Dec 20 2022, 6:11 AM
clang/lib/CodeGen/CGBuiltin.cpp
14781 ↗(On Diff #482330)

Target specific intrinsics are flexible. Some of them imply specific fast math flags. Even compiled without fast math flags, passing e.g., NaNs in this case is UB.

arsenm added inline comments.Dec 20 2022, 6:13 AM
clang/lib/CodeGen/CGBuiltin.cpp
14781 ↗(On Diff #482330)

Regardless of the behavior of the builtins, this is still not using a flag guard. It's making any instructions inserted with this Builder also have the fast math flags.

pengfei added inline comments.Dec 20 2022, 6:28 AM
clang/lib/CodeGen/CGBuiltin.cpp
14781 ↗(On Diff #482330)

Agreed. Thanks for the explanation!

arsenm added inline comments.Dec 20 2022, 7:00 AM
clang/lib/CodeGen/CGBuiltin.cpp
14781 ↗(On Diff #482330)

https://godbolt.org/z/PTseYq3Y9 demonstrates the problem. The fadd ends up with nnan implying that c cannot be nan which is not implied by the source

jcranmer-intel added inline comments.Dec 20 2022, 2:53 PM
llvm/docs/LangRef.rst
1424

I think this is trying to say that even if we know that the function is in DAZ mode, a subnormal value is not turned into poison by nofpclass(zero). But I agree with the wording being weird.

1426

Maybe mention how to combine two kinds of things into one mask, like nofpclass(sub zero)?

arsenm added inline comments.Dec 20 2022, 3:00 PM
llvm/docs/LangRef.rst
1409

I'm mostly worried about allowing clang to just throw nofpclass(nan) on every float parameter with -ffinite-math-only. What if someone is just using a floating point operand for nan boxing, and never actually performs a floating point operation on it?

arsenm updated this revision to Diff 484404.Dec 20 2022, 3:25 PM

Drop accidental diff, try to rephrase langref

pengfei added inline comments.Dec 21 2022, 1:11 AM
clang/lib/CodeGen/CGBuiltin.cpp
14781 ↗(On Diff #482330)

Yeah, but it is only a problem when user uses the builtin directly. This is not encouraged. Anyway, I put a patch D140467 for it. It's still nice to have.

efriedma added inline comments.Jan 5 2023, 2:18 PM
llvm/docs/LangRef.rst
1409

Adding nofpclass markings for -ffinite-math-only is sort of aggressive, yes. But we're already pretty aggressive in other ways, too: currently, LangRef says that passing a nan to an nnan operation produces poison.

There's maybe some argument that we should introduce some sort of weaker form of "finite-only math" that doesn't involve undefined behavior, but I haven't seen anyone push for that.

kuhar added a subscriber: kuhar.Jan 6 2023, 8:37 AM

Overall I like this direction i.e., putting a set of classes in a single attribute, which seems scalable with the number of classes and reminds me of memory effects attributes (memory(...))

llvm/unittests/IR/VerifierTest.cpp
135–136

nit: I think EXPECT_THAT(Error, StartsWith("...")) would be more idiomatic and concise. But if you prefer to follow the style used by the pre-existing checks in this file that's also fine to me.

also below

Existing decoration with nnan/ninf has inconvenient semantics. Having more powerfull and consistent solution can enable optimizations that are based on the knowledge of finitness of FP operands.

This is to help simplify the GPU builtin math library distribution.

This patch is about parameter attribute only. How would it be used? Would it be a separate tool, that analyzes bitcode, or some follow-up patches are planned?

Existing decoration with nnan/ninf has inconvenient semantics. Having more powerfull and consistent solution can enable optimizations that are based on the knowledge of finitness of FP operands.

This is to help simplify the GPU builtin math library distribution.

This patch is about parameter attribute only. How would it be used? Would it be a separate tool, that analyzes bitcode, or some follow-up patches are planned?

It just plugs into isKnownNeverNaN and co. as is handled here. The optimizations for the compares in the library should naturally fall out once the attributes are applied. The follow on patches needed are the propagation in the attributor

arsenm marked 2 inline comments as done.Jan 23 2023, 4:36 PM

ping

jcranmer-intel added inline comments.Feb 3 2023, 2:02 PM
llvm/lib/AsmParser/LLParser.cpp
2423–2425

If I'm not mistaken, these lines are unreachable?

llvm/test/Assembler/nofpclass.ll
104

A test with a hex nofpclass parameter would also be useful, given that you use that as one of the examples in the langref.

arsenm marked an inline comment as done.Feb 3 2023, 2:50 PM
arsenm added inline comments.
llvm/test/Assembler/nofpclass.ll
104

Apparently those don't parse, easier to just change the example

arsenm updated this revision to Diff 494745.Feb 3 2023, 2:52 PM

Address comments

nikic accepted this revision.Feb 23 2023, 7:33 AM

LGTM

llvm/include/llvm/IR/Attributes.h
21

Please forward-declare instead.

152

Should accept FPClassTest instead?

254

///

890

Index -> ArgNo

1252

Same here, pass FPClassTest at API level?

llvm/include/llvm/IR/InstrTypes.h
1838

Move to C++ file.

1843

Unrelated, but why isn't it a bitmask enum?

llvm/lib/IR/Attributes.cpp
472

Use ListSeparator instead.

This revision is now accepted and ready to land.Feb 23 2023, 7:33 AM
arsenm added inline comments.Feb 23 2023, 7:55 AM
llvm/include/llvm/IR/InstrTypes.h
1843

It should be, but there was a complication from LLVM_MARK_AS_BITMASK_ENUM not working for multiple enums in the same scope. 2e416cdd52c1079b8c7cb1f7d7e557c889a4fb56 was supposed to fix this but it was reverted. I'll clean this up whenever that's fixed

TimNN added a subscriber: TimNN.Feb 24 2023, 4:13 AM

The nofpclass-invalid.ll test added by this patch fails for me locally, and on the buildbot as well: https://lab.llvm.org/buildbot/#/builders/109/builds/58503

We also saw failures on nofpclass-invalid.ll and VerifierTest.cpp in our internal build/test.

Hello all,

I've seen the fix for nofpclass-invalid.ll coming down the pipeline, I'm just curious if that will fix the other failing test on this buildbot:

https://lab.llvm.org/buildbot/#/builders/216/builds/17623

It looks as though the verifier test is failing too, as pointed out by @simpal01 .

thanks for your time.

Speaking for Linaro's Arm bots, the failures have been fixed. Thanks!

You might have got a failure email for a timeout. That is for an unrelated change and I'm dealing with that.