Differential D137811
InstCombine: Perform basic isnan combines on llvm.is.fpclass Authored by arsenm on Nov 10 2022, 7:13 PM.
Details is.fpclass(x, qnan|snan) -> fcmp uno x, 0.0 Start porting the existing combines from llvm.amdgcn.class to the
Diff Detail Event Timeline
Comment Actions Rebase on added strictfp checks, and split code between InstSimplify and ConstantFolding
Comment Actions LGTM modulo inline comments.
Comment Actions Propagate poison. Leave undef alone, I still think test all/test none should win for undef
Comment Actions Also: commit description should mention the instcombine/etc changes; it makes it sound like just an amdgpu change.
Comment Actions Break up patches into 4 pieces. Assume excess bits in test mask is a verifier error, and don't handle the FP mode dependent transforms here Comment Actions
The first one sounds good. Do you have a specific motivation for the second one? Comment Actions Eventually all the class-like tests should be merged into one class call. We still want to reduce the number of tests to perform in the end and also don't want to be ordering dependent Comment Actions Early conversion is_fpclass->fcmp may result in incorrect semantics. It make the code: if (!isnan(x)) {
…
}indistinguishable from: if (x == x) {
…
}But they have different behavior if x is signaling NaN. Although the replacement is proposed for strictfp functions only, inlining may require conversion to a form that uses constrained intrinsics. The resulting code would have different behavior than original. Comment Actions
Functions which are not strictfp are allowed to introduce "spurious" fp flag writes; for example, we can flatten control flow that contains floating-point ops. Inlining the function doesn't change that general rule. The inliner converts fp operations just to ensure that later optimizations don't move those operations around. Comment Actions I didn't think we had code in the tree to convert normal FP instructions into constrained intrinsics. Andy Kaylor had a ticket with code to do this, but I didn't think it ever went in. Where is this code used by the inliner? Comment Actions
I'm not sure if that actually got merged; I was just assuming based on my memory of the discussions. In any case, it's not really relevant to the point I was trying to make. Comment Actions "spurious" fp flag writes can appear only as a result of transformations, the source code provided to compiler does not have them. Compiler should avoid such transformation at early stages of IR pipeline, otherwise it produces program with incorrect semantics. Here it is: https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Utils/CloneFunction.cpp#L390 Comment Actions
Ah, excellent. I misremembered. Thanks for the correction! Comment Actions
A transformation that’s valid for !strictfp functions cannot be invalid because of the potential for being inlined into a strictfp function. If this isn’t possible, there is a representational issue the inliner would need to account for. Comment Actions Thinking about this more, I think any situation where this could be an issue would be malformed to begin with. You had a piece of code that was expecting exceptions to be enabled calling into code where exceptions were disabled without turning exceptions off Comment Actions If exceptions were turned off, the call was made, and exceptions were turned back on then there would be no correctness issue. Inlining the called function wouldn't change that. The toggling of the exception enablement would be invisible to LLVM. Thus we can't categorically say that a strictfp function calling a !strictfp function is malformed. The constrained intrinsics that specify the default rounding and exceptions disabled are supposed to behave the same as the normal instructions. They're supposed to have the exact same meaning and behavior. So a conversion of isnan(x) to (x == x) for the constrained intrinsics in the default environment is just as correct as the instructions that do the same thing but aren't constrained intrinsics. Thus inlining of a !strictfp function into a strictfp function with the conversion of FP instructions into constrained intrinsics can't introduce any new correctness issues. Comment Actions On most targets turning off FP exceptions means reading content of FP control register, changing value of mask bits and putting the modified register value back. It is expensive operation and cannot be made invisible to LLVM. Some targets (like RISCV) do not have possibility to mask FP exceptions at all, so at IR level there is no way to turn exception off. Default FP environment supposes that the exceptions are ignored, not disabled. In general case they are raised always.
No matter what exception behavior is specified in constrained intrinsics call, processor instruction will raise FP exceptions. If bits in FP environment specifies trap on Invalid exception, the check x != x may result in crash, if x is signaling NaN. The check isnan(x) in this case is safe. So replacement is_fpclass->fcmp changes semantics and cannot be used here. Particular cores may implement is_fpclass as CMP instruction, it is made by default lowering, but making such replacement in IR is incorrect. Comment Actions
Managing the FP mode is a user responsibility. If the operation in unconstrained we can assume the default floating point environment without preserved exceptions.
Floating point exception doesn’t mean trap, but that’s a possible mode. The IR semantics are not dependent on the possible set of lowerings. Ultimately between the user and codegen, this code must never have trapped and may never be transformed to a form that could trap. The replacement fcmp must not trap, but it’s not the middle end’s responsibility to ensure that. This is correct regardless of what any particular processor may do or whatever the source did. Comment Actions Ok, true, it takes executing code to change the floating point environment. Yes, there would be inline assembly or a function call to change the FP environment. LLVM would see that code because there would be IR for it. All true. But LLVM wouldn't know what the inline assembly was doing. It wouldn't recognize the function call and thus wouldn't know what it was doing. LLVM would not know the floating point environment had changed. Rephrased, the change in the floating point environment would, to LLVM, be invisible. That's the "invisible" I was referring to earlier.
Be careful of how IEEE 754 uses the same terminology that a Unix person uses, but the words have different meanings. I'm going to use the term "754 trap" to mean a trap in the IEEE-754 document's use of the term. I'm going to say "Unix trap" when a trap involves transferring of control to the OS. An FP instruction can "754 trap" but the result may just be changing the FP status bits in the environment to record that something happened. And if we are not using the constrained intrinsics, or we are using them with exceptions "ignore" and rounding "roundtoeven", then we are assumed to not be accessing the FP status bits. A CPU is allowed to always "Unix trap" and transfer control to the OS. I think you are saying that RISCV does this. But the OS is allowed to fix things up so that the application doesn't observe the CPU's trap. Indeed, in the default FP environment the OS is _required_ to hide the CPU's "Unix trap" from the application. From the application's point of view this is the same as a CPU not doing a "Unix trap" at all. In this case we can treat it the same as a CPU that doesn't trap in the default FP environment. It's true that a blind replacement of is_fpclass with fcmp would be incorrect _if_ the floating point environment is not the default environment. But if we are in the default FP environment we can assume that any CPU trap will be hidden from us by the OS and therefore is not a part of our discussion of IR correctness. Comment Actions Non-strictfp functions are assumed to be in the default FP environment, which implies there's some form of undefined behavior if you call a non-strictfp function with a non-default FP environment. The precise, formal semantics that effect this rule is of course underdefined, This is how we define default FP environment in the LangRef today:
From this definition, it seems clear to me that we are legally allowed to insert instructions that would cause FP exceptions in non-strictfp functions. To rephrase the rules (as I understand them) in somewhat more precise terms, if you call a non-strictfp function with a non-default FP environment, the values of any FP operation are unspecified, floating-point sticky bits have unspecified values, and if the dynamic FP environment is set to generate hardware traps, the act of *calling* the function is UB (that is, we are permitted to introduce FP operations that may trap in code-paths where none existed). In this understanding, converting a non-strictfp function into a strictfp function with bare FP instructions replaced with constrained intrinsics and round.tonearest and fpexcept.ignore metadata is a valid optimization, but one that narrows the possible semantics (since non-strictfp may introduce FP operations that strictfp may not). I went ahead and looked at the RISC-V specification to see what it does on FP exceptions. The RISC-V instructions only model 754 traps as sticky bits in the fcsr, and there's an explicit note that it provides no way to convert a 754 trap to a Unix trap (to use your terminology). FWIW, C itself requires that implementations provide FP exception handling as sticky-bits (this is the IEEE 754 default exception handling), with the existence of a Unix trapping mode being an allowable extension (which it declines to specify). Comment Actions This is true for rounding direction, denormal behavior or any other FP control mode. They are represented by bits in some registers, may be set/read by appropriate API functions. Exception behavior is a very different thing. There is no register that keeps "current exception behavior". It is only a compiler hint that facilitates code generation. So user has limited means to control exception behavior.
Absolutely.
It is the middle end's responsibility to make only transformations that keep FP semantics. Replacement is_fpclass -> fcmp changes the semantics and can produce incorrect programs. Ok, I see what is "invisible". Do you think inline assembly should be decorated as def/use of FP environment similar to function calls as is done in D111433 and D139549?
What about such code? int get_code(float x) {
return isnan(x) ? 1 : 2;
}
void func1(float x) {
int code1 = get_code(x);
...
}
#pragma STDC FENV_ACCESS ON
void func2(float x) {
int code1 = get_code(x);
...
}Acoording to C standard it must work as intended. If compiler replaces isnan with comparison, the code would be broken. LLVM must be able to represent semantics of the supported languages. The code snippet above represents a program that is valid from viewpoint of C standard but would be broken if the middle end makes replacement isnan(x)->x!=x. So such replacement must not be made. Comment Actions It doesn't change the semantics because the FP exception that the machine fcmp may end up raising can be assumed to be invisible based on the IR semantics. An unconstrained fcmp does not have observable FP exceptions. For any regular non-constrained operation, you simply never need to be concerned with floating point exceptions. They can't be observed or relied on.
This code didn't actually manipulate the floating point mode, just made it legal to do so for the scope of the pragma. If you were to insert code to modify the mode to enable FP exceptions inside func2, it would be illegal because you did not restore the mode before calling into code outside of the pragma scope. FENV_ACCESS doesn't require the compiler to fixup the floating point mode for you before calling into other code. Comment Actions From the C standard (7.6.1p2 from draft N3054):
On a strict reading of the standard, on transition from get_code (which has FENV_ACCESS OFF) back to func2, the FP flags are unspecified, which means it is legal to transform the non-flag-setting isnan into flag-setting fcmp. There is a slight wording mismatch, as FENV_ACCESS is about testing, not setting flags, but the footnote seems to indicate that the correct reading is that FENV_ACCESS is necessary if you want guarantees about flags being set or not set. If it would help, I could contact the CFP study group to see what they think is the correct reading of the standard. Comment Actions For the purpose of this change, the exact definition of FENV_ACCESS doesn't matter. This is not something that the context of a single instruction can or should consider. This would have to be managed by implicit mode switches inserted by the frontend to maintain the invariant that non-strict functions don't have to be concerned about it. Our strictfp representation would be totally broken if we had to think about this in a non-strict function Comment Actions The underlying question is if it's legal for a compiler to insert a call to a flags-setting instruction in FENV_ACCESS OFF along code paths that never had them (with the answer almost certainly "yes"). We have these semantics for non-strictfp in LLVM. Comment Actions Pragma STDC FENV_ACCESS ON may be used not only for code that manipulates control modes, but also when exception status is examined (7.6.1p2): The FENV_ACCESS pragma provides a means to inform the implementation when a program might access the floating-point environment to test floating-point status flags or run under non-default floating-point control modes. A call to fetestexcep could be put somewhere inside func2 to have an access to FP environment, but it does not change the behavior of isnan inside get_code. This statement is not pertinent to this case because the only operation in get_code is a call to isnan, but it must not raise any exception according to the same standard (F3p6): The C classification macros fpclassify, iscanonical, isfinite, isinf, isnan, isnormal, issignaling, issubnormal, iszero, and signbit provide the IEC 60559 operations indicated in the table above provided their arguments are in the format of their semantic type. Then these macros raise no floating-point exceptions, even if an argument is a signaling NaN. and this property must be kept no matter if exceptions are ignored or not. In a broader context, it looks natural that a non-strictfp function may lose some exceptions that strictfp function would raise. But it would be counterintuitive if it raised new exceptions, which cannot be observed in strictfp function. Comment Actions I do not believe there's any problem with an isnan to fcmp transform -- as long as we restrict it to non-strictfp functions. And I'm not sure why there's so much debate here. I thought all those sorts of semantic questions were already pretty well settled. Does the documentation about strictfp vs not-strictfp semantics need to be made clearer? No, the property does not need to be maintained if the exception flag state cannot be observed by correct code. And that's exactly the case at hand in FENV_ACCESS OFF code (and, correspondingly, non-strictfp in LLVM IR). In such code, you are not allowed to have trap-on-exception enabled, nor are you allowed to access the floating-point exception status flags, and, the state of the status flags is unspecified upon transitioning back to FENV_ACCESS ON code. Thus, within such code, we may set the status flags to any arbitrary value we wish -- nobody can (correctly) tell the difference.
That's not a problem. It is definitely permissible for a non-strictfp function to raise an FP-exceptions that "should not" be raised. That this is permissible is one of the most important properties of the non-strict operations -- it allows code motion which is impermissible if the status-flag side effects must be taken into account. (e.g. speculating an "fadd" above a conditional).
Comment Actions What about the original case, when isnan is used in inline function? inline int get_code(float x) {
return isnan(x) ? 1 : 2;
}
#pragma STDC FENV_ACCESS ON
void func2(float x) {
int code1 = get_code(x);
...
}Can this function, defined as non-strictfp, raise Invalid exception when inlined into strictfp function?
There is a use case, when a computation is made with traps allowed for errors (Invalid, Overflow, DivideByZero). If an error occurs, computation is stopped. It saves from checks in multiple places during computation. The computation runs in default mode, because rounding mode is standard and precise exception flag behavior is unimportant. If non-strictfp functions are allowed to throw spurious exceptions, such solution won't work. Running computations in strictfp mode is not an option because of poor performance. Comment Actions Inlining cannot change semantics. The function can, but not from the part derived from the inlined callee Comment Actions Since the function is non-strictfp, it's legal to optimize it in a way to raise spurious exceptions. If the isnan is retained at the time the function is inlined into a strictfp function, then it is no longer possible to optimize the copy of isnan in the strictfp function to raise spurious exceptions. But an optimization pass that runs before the inliner may still introduce spurious exceptions.
Enabling hardware traps for FP exceptions makes the FP environment no longer in the default mode, which requires the computation to be done in a strictfp function to have any defined behavior. Comment Actions As far as I can tell, this conversation has run its course, and I don't think there were any more comments on the actual code, so I'm going to accept this revision. Use-cases that require traps or otherwise observing floating-point status flags must generate strictfp functions and constrained intrinsics. (E.g. in Clang, you might wish to compile with -ffp-exception-behavior=maytrap). That was true before this change, too. If the performance of strictfp code is not as good as desired, then I'm sure there's more work that can be done optimizing it -- but there is a reason it's a separate mode. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
I don't think this function should be added -- it emits the wrong thing for strictfp mode, but also just doesn't seem useful.
That we sometimes canonicalize to fcmp is just an implementation detail -- the CreateFCmpUNO call can just be done inline in foldIntrinsicIsFPClass.