[PATCH] D140294: clang: Replace implementation of __builtin_isnormal

Tue Dec 20 04:10:38 PST 2022

arsenm added a comment.

In D140294#4007073 <https://reviews.llvm.org/D140294#4007073>, @sepavloff wrote:

> This change can have negative consequences in some cases. Some targets have dedicated instruction to test FP class and often this instruction is faster than arithmetic operations. Replacement of one operation with two arithmetic and two logic plus cost of FP constant materialization does not provide any visible benefit.

Not sure what you're talking about, there's no class call here. Besides, optimal codegen is not clang's concern

> One of advantages that llvm.is_fpclass provides is the possibility for a target to use FCLASS instruction if it is available. Besides, llvm.is_fpclass incapsulates semantics of a class check, which is not identical to compare operations. Keeping the class checks in IR can facilitate FP-specific optimizations which provide different handing depending on possible FP classes. Fast math is one example, denormals is another.

It's almost but not quite the same. The middle end can swap back to fcmp as long as we don't have FP exceptions to worry about (in the denormal case we can reform compares against 0/smallest_normal depending on the known denormal mode). I have patches ready to swap between all of these cases

> I would propose to generate llvm.is_fpclass for all classification intrinsics (see D137811 <https://reviews.llvm.org/D137811>). Targets then may lower the intrinsic in a way most suitable way depending on ISA.

Right, I think clang emitting is.fpclass and letting the middle and and backend fold back into fcmp as legal is a reasonable strategy. In cases surrounding denormals we just need to know the floating point mode. The possible downside to always emitting class upfront in clang is you immediately lose fast math flags (I'm not entirely sure having fast math flags on fcmp is a good idea though)

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140294/new/

https://reviews.llvm.org/D140294