Handle denormal input for fcmp instructions according to denormal float handling mode.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Thanks, I'd started looking at how to include fcmp but you were quicker.
llvm/lib/Analysis/ConstantFolding.cpp | ||
---|---|---|
1345–1347 | If the denormal mode is used as an argument instead of a function pointer to obtain it, then DenormMode.Input or DenormMode.Output can be used directly instead of the mode struct plus a selector. | |
llvm/lib/Analysis/InstructionSimplify.cpp | ||
3896–3897 | The SimplifyQuery Q has a pointer to the instruction, so the denormal mode can be obtained via its parent rather than needing the denormal mode as an additional argument. | |
3903–3904 | I feel it would be better to handle the denormals inside ConstantFoldCompareInstOperands (it would only need the instruction pointer from Q) as that would correct the result for all compare folding, whereas this patch currently only corrects the call from InstSimplify. |
Thanks for review @dcandler . Updated accordingly.
llvm/lib/Analysis/ConstantFolding.cpp | ||
---|---|---|
1345–1347 | Good point. Done. | |
llvm/lib/Analysis/InstructionSimplify.cpp | ||
3896–3897 | I chose not to use SimplifyQuery Q before is because the instruction in Q may be nullptr. Seems using the instruction inside Q also does not impact the motivated case, so I use it now. | |
3903–3904 | Sure. I change to handle denormal input in ConstantFoldCompareInstOperands now. However ConstantFoldCompareInstOperands is not just for float compare, it is also for integer compare. So it also makes to me that we don't add a parameter specific for float in the function. |
llvm/lib/Analysis/ConstantFolding.cpp | ||
---|---|---|
1386 | Prefer dyn_cast rather than isa+cast. So it could be written like this: // Flush denormal inputs if needed. if (auto *LCFP = dyn_cast<ConstantFP>(LHS)) { const fltSemantics &FltSema = LCFP->getType()->getFltSemantics(); LHS = FlushFPConstant(LCFP, F->getDenormalMode(FltSema).Input); } if (auto *RCFP = dyn_cast<ConstantFP>(RHS)) { const fltSemantics &FltSema = RCFP->getType()->getFltSemantics(); RHS = FlushFPConstant(RCFP, F->getDenormalMode(FltSema).Input); } // Calculate constant result. Constant *C = ConstantFoldBinaryOpOperands(Opcode, LHS, RHS, DL); // Flush denormal output if needed. if (auto *CFP = dyn_cast<ConstantFP>(C)) { const fltSemantics &FltSema = CFP->getType()->getFltSemantics(); C = FlushFPConstant(CFP, F->getDenormalMode(FltSema).Output); } return C; | |
llvm/test/Transforms/InstSimplify/constant-fold-fp-denormal.ll | ||
762–763 | Please add more tests to verify that we have the expected behavior for other denormal modes. These would be similar to what was added with D116952 (see tests in this file above here). | |
772–773 | This is identical to the previous test? |
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
3896–3897 | In the previous review the view was that if there is no parent instruction/function available to get the denormal mode from, then it is okay to default to IEEE. | |
3903–3904 | ConstantFoldCompareInstOperands does do both float and integer compares, so my thinking was to only start worrying about denormals inside that function after we can establish whether it is dealing with integer or float instructions. That was why I suggested using an instruction pointer as an additional parameter: regardless of the type, a pointer to the instruction being folding should be available in most cases, and then this can be refined down to the denormal mode within the function only when needed. This would make it easier to update calls to ConstantFoldCompareInstOperands elsewhere (e.g. ConstantFold in SimplifyCFG.cpp), since the existing instruction pointer can just be passed down, rather than having to determine a denormal mode for every usage, especially when a denormal mode may not be needed. | |
llvm/test/Transforms/InstSimplify/constant-fold-fp-denormal.ll | ||
1109–1111 | Since compares only take floating point inputs, another relevant test would be to ensure compares don't get folded when only an output mode is set (attributes #1 and #2) but do get folded on an input mode (#3 and #4). |
llvm/include/llvm/Analysis/ConstantFolding.h | ||
---|---|---|
69–77 | Since we are updating this comment, please remove the function name at the start: Also, change the wording slightly: For fcmp, denormal inputs may be flushed based on the denormal handling mode. | |
llvm/test/Transforms/InstSimplify/constant-fold-fp-denormal.ll | ||
769 | Here and in other tests - I didn't recognize the hex value as a denorm at first sight. It would be easier to see with the leading zeros shown explicitly. | |
772–773 | Is this still the same as the previous test? It would be better to make the denorm constant as operand 0 and maybe vary the fcmp predicate for better coverage. |
denormal -> Denormal