Ported from old amdgcn intrinsic which will soon be deleted.
Details
Diff Detail
Event Timeline
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
6108 | Why is this not strictfp-safe? |
I think you can simplify ldexp(x, C) -> x * ldexp(1.0, C). Even for strictfp this should work if you use a constrained fmul and the if the new ldexp itself does not overflow or underflow.
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
6108 | It would pass through an SNaN instead of quieting it I expect. |
llvm/lib/Analysis/ConstantFolding.cpp | ||
---|---|---|
2710 | This is dead code with the extra case you added above. | |
llvm/lib/Analysis/InstructionSimplify.cpp | ||
6084 | Why is this not strictfp-safe? | |
6106 | Why is this not strictfp-safe? Maybe I just need a good description of what strictfp implies. The description in the langref mentions rounding mode, status flags and trapping, but says nothing about quieting NaNs. |
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
6084 | If undef resolved to a signaling nan it wouldn't raise an exception | |
6106 | A signaling nan is supposed to raise an exception which quieting it would hide. The LangRef states signaling nans may not be quieted by non-constrained operations and constrained should handle them properly |
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
6084 | But here you are choosing what you want the undef value to be, so choose a quiet NaN. |
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
6084 | This is checking this which is a more refined check of strictfp |
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
6084 | although really I should probably just call simplifyFPOp in the first place |
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
6084 | Well I don't understand why that code doesn't propagate quiet NaNs unconditionally. I agree with @sepavloff's comment: https://reviews.llvm.org/D103169#inline-979968 Also, that code handles all fp ops but here we only care specifically about ldexp. Where is the spec for what fp exceptions ldexp can raise? man ldexp mentions exceptions on overflow and underflow, but does not mention raising invalid operation even on a signalling NaN input. In any case a comment explaining why each case is supposedly not fpstrict-safe would really help, since this stuff is massively non-obvious. |
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
6084 | man ldexp says:
| |
6084 |
This is implied for every FP operation. There are just the exceptions for fabs/fneg/copysign/is.fpclass |
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
6106 | With "maytrap' we are allowed to remove exceptions. That should make quieting an sNaN safe, no? Also, are callers checking for the default fp environment? That should behave the same as non-constrained operations. |
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
6106 | I just remembered this also has the problem that the constrained operations aren't fully expressive enough. "Default FP environment" doesn't cover denormal flushing or other target dependent modes. |
Drop dead code, add strictfp and FMF todo. I looked into merging with simplifyFPOp, but it would multiply the complexity of the patch. I also don't necessarily agree we have adequate information to fold these given denormal input exceptions exist and the denormal mode is dynamically changeable
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
6100 | Also handle qNaN here? |
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
6100 | Technically would add more brokenness to old mips signaling nans although I don’t think we have a ruling on how much we should care |
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
6100 | Isn't this a problem that should be solved in APFloat? Anyway, it seems like we shouldn't let old mips keep us from optimizing in the present. |
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
6100 | APFloat would need to know what to do and maybe treat it as a separate type. Maybe it should be part of DataLayout, I don’t know |
The point of the patch is to optimize the regular version. I handled the strictfp parts that do not require any thought. I don't want to further complicate this step for strictfp
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
6084 | simplifyFPOp complicates things because that's expecting only float inputs and here there's an integer op |
Fix comment
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
6084 | I keep trying to make it use simplifyFPOp and I'm unhappy with it. It's ignoring the denorm flushing potential, uses FastMathFlags and i'm about 80% sure the precedent here for getting to the FMF is broken. I see various places using the CxtI in the SimplifyQuery, which is likely not the instruction the flags are actually attached to. It's not really less code to just handle the nan case directly while I have the APFloat |
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
6084 | I've read all the comments here and I still don't understand why this case is not strictfp-safe. |
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
6084 | Folding to the input operand drops a canonicalize. It would be more correct to introduce llvm.experimental.constrained.canonicalize which does not exist |
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
6084 | You have a free choice of what input operand value to assume, so choose one for which llvm.experimental.constrained.canonicalize would be a no-op? Isn't that already true for the value returned by getNaN? (Or is this some MIPS weirdness again where we don't know whether that NaN is quiet or not?) |
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
6084 | Maybe for the ldexp(undef, x) -> nan case (where we evidently don't have concrete rules for payload bits) I'm talking about the ldexp(x, undef) -> x case |
ping, I'll drop strictfp support completely and never come back to it if it moves this along
I still think removing support for amdgcn_ldexp should be in the patch that removes amdgcn_ldexp.
I still think every simplification guarded by !IsStrict should have a comment saying why, even if it's just "to be conservative because we're not *sure* that it's fpstrict-safe".
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
6083 | I still don't understand why this one isn't strictfp-safe, if you simplify -> qnan. |
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
6083 | if undef could be anything, it could have been a signaling nan that would demand quieting |
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
6083 | Plus we evidently don't have agreement on how nan payload bits are supposed to work |
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
6083 |
That's like saying "it could have been 99.9". The point is we are *choosing* a particular value to refine it to, so why not choose a quiet nan?
OK, if we are unable to make a quiet nan because we don't know what bits to put in the payload, that seems like a good reason - but please add a comment to that effect since it is massively non-obvious. (Also if that is true then how does C->makeQuiet() below work??) |
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
6083 | Make quiet just flips the bit of an existing nan which is obviously ok (ignoring old mips). This is synthesizing a new choice |
Just fold the undef for strict, this is near universally broken anyway if it's decided it's broken
Should keep support for the old intrinsic while it still exists.