Consider floating point environment flags when constant folding
expressions.
Details
- Reviewers
• tstellarAMD mehdi_amini hfinkel
Diff Detail
- Repository
- rL LLVM
Event Timeline
include/llvm/Analysis/ValueTracking.h | ||
---|---|---|
261 | I think we should leave the one-sentence intro as-is to maximize readability; it's still correct with the new settings, AFAICT. Then let's add a paragraph to explain the floating-point-specific details: "If KeepExceptions is true, then speculative execution of most floating-point operations will result in observably different floating-point exceptions being raised." Worth noting: it's not obvious that KeepRounding fits the bill for isSafeToSpeculativelyExecute, but rounding-mode does change the observable exceptions by moving the overflow/underflow boundaries. However, that's just one small part of what you're trying to get at here with rounding, the larger part of which seems conceptually to be closer to "memory dependent" (except that the "memory" is floating-point control). |
Updated text of comments, updated code to pass fast-math flags in several places, adjusted code according to review comments, updated tests which are affected by inversion of meaning of flags.
Revert questionable change in undef-label.ll (it shouldn't be here anyway), this also removes four tests that are now in D14067.
include/llvm/Analysis/ValueTracking.h | ||
---|---|---|
265 | The KeepExceptions parameter is not clear to me, what would it be used for? (there is no use in this patch) | |
include/llvm/IR/Constants.h | ||
1106 | Document the new FMF parameter, especially how it differs from Flags, it is confusing right now. Also, seeing multiple places where you need to explicitly add 0, nullptr, to be able to pass a FMF, it can be worthwhile to add an override that does not take Flags and OnlyIfReducedTy and forward to this one. | |
1243 | Should it be allowed on non-FP operations? I could see it assert instead. | |
lib/Analysis/InstructionSimplify.cpp | ||
3783 ↗ | (On Diff #41912) | It this related to this patch or could this be committed separately? |
lib/Analysis/ValueTracking.cpp | ||
3400 | You already tested for KeepExceptions at this scope. | |
lib/IR/ConstantFold.cpp | ||
1182 | I'd refactor the test with a named lambda defined before the switch. | |
lib/IR/Constants.cpp | ||
1804 | You may want to assert that if FMF is supplied we have a Floating point Opcode? | |
lib/Transforms/InstCombine/InstCombineMulDivRem.cpp | ||
1214 | Initialize closer to its use, there are multiple path that early return and don't need this. | |
lib/Transforms/InstCombine/InstructionCombining.cpp | ||
215 | No braces |
@joker.eph, thanks. Changed according to your comments, but tests on this revision don't pass... Can't reorder commits nicely due to changes of flags (now that default value FastMathFlags() is "bad" for incremental changes). Instead I'm leaving patches separated for review, but when committing will squash them to make tests pass on each revision.
include/llvm/Analysis/ValueTracking.h | ||
---|---|---|
265 | It's left from one of previous versions, the one with function attributes. Removed. | |
include/llvm/IR/Constants.h | ||
1106 | Should be done, at least for this patch. | |
1243 | Moved it to place where it's used (used to be two such places), changed type of argument and added an assert. | |
lib/Analysis/ValueTracking.cpp | ||
3400 | Accidentally applied stash to the next patch instead of this one. | |
lib/IR/ConstantFold.cpp | ||
1182–1183 | Changed it using Optional. |
include/llvm/IR/FastMathFlags.h | ||
---|---|---|
38 | Shouldn't the default be NoExceptions | NoRounding ? |
Thinking about it more we will never reach correctness without having the safe option by default.
I think we should leave the one-sentence intro as-is to maximize readability; it's still correct with the new settings, AFAICT. Then let's add a paragraph to explain the floating-point-specific details:
"If KeepExceptions is true, then speculative execution of most floating-point operations will result in observably different floating-point exceptions being raised."
Worth noting: it's not obvious that KeepRounding fits the bill for isSafeToSpeculativelyExecute, but rounding-mode does change the observable exceptions by moving the overflow/underflow boundaries. However, that's just one small part of what you're trying to get at here with rounding, the larger part of which seems conceptually to be closer to "memory dependent" (except that the "memory" is floating-point control).