[PATCH] D149715: IR: Add llvm.frexp intrinsic

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff added inline comments.


================
Comment at: llvm/docs/LangRef.rst:14767
+This intrinsic splits a floating point value into a normalized
+fractional component and integral exponent.
+
----------------
integral -> integer?


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h:1831
 
+  /// Build and insert \p Fract, \p Exp = G_FLDEXP \p Src
+  MachineInstrBuilder
----------------
G_FLDEXP -> G_FFREXP?


================
Comment at: llvm/include/llvm/IR/Intrinsics.td:1043
+  // TODO: Should constrain all element counts to match
+  def int_frexp : DefaultAttrsIntrinsic<[llvm_anyfloat_ty, llvm_anyint_ty], [LLVMMatchType<0>]>;
 }
----------------
Can `LLVMScalarOrSameVectorWidth` be used to constrain return values?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:2086
+    Entry.IsSExt = TLI.shouldSignExtendTypeInLibCall(ArgVT, isSigned);
+    Entry.IsZExt = !TLI.shouldSignExtendTypeInLibCall(ArgVT, isSigned);
+    Args.push_back(Entry);
----------------
Can it be just `Entry.IsZExt = !Entry.IsSExt`?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:2504
+
+  SDValue NegSmallestNormalizedMask = DAG.getConstant(
+      APFloat::getSmallestNormalized(FltSem, true).bitcastToAPInt(), dl,
----------------
The variable name is misleading. It is not a mask, it does not select bits. It is just a value.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:2513
+  // Masks out the exponent bits.
+  SDValue InfMask =
+      DAG.getConstant(APFloat::getInf(FltSem).bitcastToAPInt(), dl, AsIntVT);
----------------
`ExpMask` could be a better name, there is no infinity bit.


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

https://reviews.llvm.org/D149715