[PATCH] D128647: [InstructionSimplify] handle denormal constant input for fcmp

[David Candler via Phabricator via llvm-commits]

dcandler added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:3896-3897
 static Value *simplifyFCmpInst(unsigned Predicate, Value *LHS, Value *RHS,
                                FastMathFlags FMF, const SimplifyQuery &Q,
-                               unsigned MaxRecurse) {
+                               unsigned MaxRecurse, DenormalMode DenormMode) {
   CmpInst::Predicate Pred = (CmpInst::Predicate)Predicate;
----------------
shchenz wrote:
> dcandler wrote:
> > 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.
> 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.
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.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:3909
+                               /* IsOutput */ false);
       return ConstantFoldCompareInstOperands(Pred, CLHS, CRHS, Q.DL, Q.TLI);
+    }
----------------
shchenz wrote:
> dcandler wrote:
> > 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.
> 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.
`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.


================
Comment at: llvm/test/Transforms/InstSimplify/constant-fold-fp-denormal.ll:789
 attributes #5 = { nounwind "denormal-fp-math"="ieee,ieee" "denormal-fp-math-f32"="positive-zero,ieee" }
 attributes #6 = { nounwind "denormal-fp-math"="positive-zero,positive-zero" }
----------------
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`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128647