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

[ChenZheng via Phabricator via llvm-commits]

shchenz marked an inline comment as done.
shchenz added a comment.

Thanks for review @dcandler . Updated accordingly.



================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:1349
 // their mode set separately, so the direction is also needed.
-Constant *FlushFPConstant(Constant *Operand, const llvm::Function *F,
-                          bool IsOutput) {
-  if (F == nullptr)
+ConstantFP *llvm::FlushFPConstant(ConstantFP *Operand, DenormalMode DenormMode,
+                                  bool IsOutput) {
----------------
dcandler wrote:
> 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.
Good point. Done.


================
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;
----------------
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128647