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

Thu Jun 30 04:21:56 PDT 2022

spatel added inline comments.

================
Comment at: llvm/include/llvm/Analysis/ConstantFolding.h:70-74
 /// ConstantFoldCompareInstOperands - Attempt to constant fold a compare
 /// instruction (icmp/fcmp) with the specified operands.  If it fails, it
 /// returns a constant expression of the specified operands.
-///
-Constant *
-ConstantFoldCompareInstOperands(unsigned Predicate, Constant *LHS,
-                                Constant *RHS, const DataLayout &DL,
-                                const TargetLibraryInfo *TLI = nullptr);
+/// For fcmp, denormal constant input will be folded further according to the
+/// denormal handling mode.
----------------
Since we are updating this comment, please remove the function name at the start:
"Don’t duplicate function or class name at the beginning of the comment."
https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments

Also, change the wording slightly:
  For fcmp, denormal inputs may be flushed based on the denormal handling mode.

================
Comment at: llvm/test/Transforms/InstSimplify/constant-fold-fp-denormal.ll:769
 entry:
   %cmp = fcmp une double 0x0, 0x8000000000000
   ret i1 %cmp
----------------
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.

================
Comment at: llvm/test/Transforms/InstSimplify/constant-fold-fp-denormal.ll:773

 define i1 @fcmp_float_positive_zero() #6 {
 ; CHECK-LABEL: @fcmp_float_positive_zero(
----------------
spatel wrote:
> This is identical to the previous test?
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128647